update QUnit, use Karma, make some of CI work locally as well

[Krinkle]

• Load qunit package from npm. Start of a larger transition, ref Introduce npm+webpack to build the app #554.

• Update QUnit from 2.3 to 2.12.

• Use Karma for the running of unit tests (instead of Nightwatch).

  While it was possible to use a fake "UI" test to run a QUnit web page with Nightwatch, this was not ideal. This had numerous limitations, such as:

  • relies on fragile an unsupported web scraping to collect results, which can easily break between versions.
    ref qunit updated #660.
  • results provide limited debugging information when a test fails.
  • cannot easily be run locally from the command-line since the Nightwatch config is currently written for SauceLabs, and creating a local configuration is not easy because Nightwatch has a hard requirement on webdriver (which makes sense for UI tests), which people usually do not have installed or may be incompatible with the current version of their browser. These then also have to be configured and started etc.
  • cannot easily be run in a secure container separate from your personal computer, thus putting personal data at risk.
  • lacks wider integration and plugins to enrich unit testing, such as test coverage reports.

• Using Karma means:

  • You can run 'npm test' locally during development and have it automatically run the tests in headless Firefox and Chrome and report back, all from the command-line.
  • The same exact runner is also used in CI with SauceLabs for additional browser coverage (same as before).
  • It has no external dependencies other than the plain web browser itself. This means if you have a development container (e.g. based on Docker) that has Node.js + Firefox + Chromium, you can run the tests from within there without exposing anything from your personal computer, except the current working directory.
    https://timotijhof.net/posts/2019/protect-yourself-from-npm/
  • In a future change, we can plug in karma-coverage to generate a test coverage report, and then submit this to Codecov or Coveralls, ref Introduce code coverage with Codecov #528.

• I have restructured the Travis CI file so that the cross-browser test in SauceLabs are skipped for PRs from forks. Instead, those PRs will run the unit tests in local Fireox and Chrome within Travis CI. This avoids the confusing failures and still gives (some) useful results.

  The tests that Travis CI runs are the same as what users can run locally if they invoke npm test (after a one-time run of npm install to fetch dependencies into the local node_modules directory).

  This restructuring uses the 'stages' feature which is not compatible with the 'deploy' feature. I had converted the two deploy stages to use the 'stages' feature as well so that these continue to work.

• I have pinned the version of 'http-server' and 'nightwatch' in package.json so that these don't silently switch major versions that may contain breaking changes.

Fixes #653.

[Krinkle]

In my fork, I have pushed a second branch in which I have this commit with the modification to enable the "SauceLabs" stage even for forks, and added my own SauceLabs credentials via Travis CI. This to confirm that this stage also works as intended:

https://travis-ci.com/github/Krinkle/kiwix-js/builds/203361391

[Jaifroid]

@Krinkle This looks excellent to me. I haven't done any testing. Do we need central credentials for Karma? My only other comment, as you will have noticed, is to add edge87 or edge86 if available on the testing platform.

[mossroy]

Many thanks for this PR!
Thanks for the improvement on Travis build on forks : having unit tests is always better than no CI at all

I thought that Karma would replace nightwatch even for UI tests, but I probably misunderstood. The code coverage on current unit tests alone would not be very relevant

[mossroy]

I suppose this is not true any more : unit tests need qunit from node_modules directory, and they still need to go through a local webserver.

[Krinkle]

The way I've run these ad-hoc is by executing php -S localhost:4000 or python3 -m http.server 4000 to start a static server from the repository directory. These are (while using somewhat unrelated technologies) are rather ubiquitous, simple, and dependency-free.

[Krinkle]

I removed "some tweaks" since I encountered no issue running these in Chrome. I'm not sure what these tweaks referred to, but I guess there was an issue at some point that required modifying some of the files?

I'm happy to revert this change. Did I misunderstand?

[Jaifroid]

I think the tweaks referred to running the file from file:// (--allowfileaccessfromfile or something like that), but this has largely been blocked by all but legacy browsers. So it's fine to remove the reference to tweaks.

[mossroy]

I agree to remove the tweaks reference, but we should mention here what is necessary to do for a newcomer : if a npm install is necessary or not, and that it must be opened through a local webserver (and suggest the php and python ways)

[mossroy]

If npm install is now necessary, it also should be mentioned at the beginning of the body of tests/index.html

[mossroy]

Regarding the code coverage, we should probably add more unit tests.
In my paid job, I advocate to have a strong unit test coverage (that run quickly and reliably, on the developer workstation), and rely less on E2E tests : it might be time to apply that on kiwix-js :-)
But it's probably not easy, and might need to refactor our monster app.js to make it more testable

[Krinkle]

If I understood correctly, Karma is used for unit tests only (with QUnit), and nightwatch is kept for UI tests?
Do you think it would be possible to use Karma for both?
Else the karma code coverage would not be very relevant.

In my understanding, it is usually not wanted for end-to-end UI tests to have code coverage tracked. Are you sure you want that? Or could perhaps some of the UI tests be better written as an integration test within QUnit? It depends on how modular the code is, e.g. if part of it can be instantiated at will and then torn down again.

For end-to-end UI tests this would be quite hard. They tend to need multiple concurrent browser sessions, which would need to be aggregated, and capturing at the end of each session will not work if the test involved a form submission or other navigation or reload. Code coverage generally works by transforming every branch of every statement in the code with a wrapper that tracks the offset and original file name in a large tree stored under a global variable (e.g. Istanbul, NYC, V8, etc.) - which would be lost upon browser navigation.

Having said that, for an SPA where there are no "real" browser navigations - this might work. But, I get the impression that Kiwix is moving toward PWAs for a more stable user experience, lower memory use, faster performance, etc.

I've looked into Nightwatch.js and WebdriverIO, as popular webdriver frameworks, and from what I can tell they don't really have a good way to do this. But.. it's not impossible. The lower-level libraries for Instanbul code instrumentation could be used directly in a custom script for Kiwix, and then we'd need a version of the www app that serves those files instead, and then before every click and navigation we'd somehow serialize and send this to a (separate) Node.js local server, combine each of the files, and then write it to NYC to create the coverage summary reports.

But the way I would normally do this elsewhere is for most tests to be to use QUnit for both unit tests (no shared state, observe one class or function at a time, inject/construct any needed state) and integration tests (some shares state, drive the main application). And then use end-to-end test with webdriver to validate a small number of scenarios for each high-level feature to confirm that the (nicely separate) UI code works correctly, which would be excluded from the code coverage percentage.

And it also duplicates target browsers definitions

Acknowedged, this is a small cost to make the most of UI tests and unit tests in their own right. Unfortunately, while they are similar, the needs for each are different enough that the industry has generally solved them in different ways.

For UI tests, it is expected that a web application and server already exists and are launched in their "normal" way, no modifications or special instrumentations. And we launch the browser through a remote control "webdriver", which requires extra software or bridges to be installed (geckdriver, chromedriver, etc.). Capturing code coverage from here is hard. But, you can navigate around a site from a single test case and perform UI actions in a realistic way. E.g. typing into text fields is simulated the way a user would do it.

For unit tests, there is no web app or server at play. They run either in Node.js itself, or in an empty browser window. Source code is loaded, and then the program executes in a non-visual way. Classes are instantiates, functions are called etc. At the end of it all, coverage is collected from the (one, empty) browser window in which all tests ran, and you have a coverage report. All code executes client-side. There is no webdriver or remote control involved. These tests run generally much faster as there are no reloads and no intermediary remote controls, you could afford to have hundreds of variations of a specific interaction to ensure all edge cases are covered without it taking very long.

[Krinkle]

[…] Do we need central credentials for Karma?

No, this continues to use the same SauceLabs credentials, and will read them from the same secure env variables as before.

[mossroy]

Thanks for your feedback on tests @Krinkle
It seems to correspond to the thoughts I had afterwards in #672 (comment)
I agree with you that trying to measure code coverage on UI tests is a bad idea.
What we should do is measure the code coverage on karma (unit tests) : this coverage will be disappointing, which should push us to do more unit tests.

Regarding the browsers configuration duplication, it's not a big issue to keep it as it does not evolve frequently : it should be enough to put a comment in both places to remind to keep them in sync

[Krinkle]

Thanks, I'll rebase and amend for the above points next weekend.

[mossroy]

Apart from my latest minor comments, it looks good to me.
Merging this would allow to test the CI in real conditions

[Krinkle]

@mossroy I thought I had addressed them all. Can you remind me which ones I missed?

[mossroy]

@Krinkle : I was mentioning https://github.com/kiwix/kiwix-js/pull/672/files#r532174960 and https://github.com/kiwix/kiwix-js/pull/672/files#r532173980 (I just unresolved this conversation)

[Krinkle]

Oops, I hadn't seen those indeed. Will do!

[kelson42]

Branch needs to be rebased as well.

[Jaifroid]

@kelson42 I think the only conflict here is the .travis.yml file, from which we have removed the Continuous Integration code, since it is now handled by GitHub Actions. Since Travis has stopped working entirely for us due to their decision to start charging Open Source projects (and we have no credit left), we need to re-evaluate with @Krinkle which parts of this PR can be transferred to GitHub Actions. The underlying changes for QUnit are still extremely useful for us.

[Krinkle]

@Jaifroid Do I understand correctly that the unit tests have been disabled currently? I don't find the analogous code in the .github/ directory currently to re-apply this change to.

I can help with that transition as well, but someone will need to create the necessary "Secrets" behind this repository for Sauce Labs.

Also, make sure action workflows are enabled for fork PRs, but with secrets disabled. See blog post for more info.

[Jaifroid]

@Krinkle Yes, unfortunately Travis is completely blocked due to a negative credit balance (caused by a policy change on the Travis side). I have not attempted to write equivalent code to launch the tests on GitHub Actions because I am unfamiliar with the operation of Unit Tests and I'm aware of potentially conflicting with this PR. I did manage, with @mossroy's help, to write CI.yml which launches build scripts for nightly and release builds.

If you are able to help with the translation for tests on pull requests and commits, that would be fantastic. @mossroy or I can help with secrets.

[Jaifroid]

@mossroy I have a login/pw for Sauce labs, but not a private key secret. Do you have that?

[mossroy]

I have a Sauce username and "Access Key", that I can add as Github Actions secrets

This repo is public so, based on the github blog, we might be able to use pull_request_target to let the CI run on a fork PR

[mossroy]

I've just added SAUCE_USERNAME and SAUCE_ACCESS_KEY secrets (same names as the corresponding Travis-CI secrets, and normally same values)

[Krinkle]

@mossroy Thanks, I think I've got it all figured out now. A ran into a lot of issues, which I've tried to document as best I can.

In particular:

• On Travis, when creating a pull request secrets are naturally left out and the secure job can be skipped as being a fork-PR. When you run Travis jobs on commits and branches directly in your forked repo (not a PR), you have control and can provide your own secrets, which makes forking and testing easy for contributors. GitHub, workflows on a forked repo are always executed from the upstream perspective even if it is just a push in a local branch and not a PR. This seems like a bug to me, and is made worse by the following:
• GitHub does let you configure secrets in a forked repo, but this is a dead-end since they are completely ignored. No warning or error.
  • I worked around this by temporarily creating an unmarked fork at https://github.com/Krinkle/kiwix-js--fork. I realize now I should have just worked in a local branch here. I forgot you added me already, oh well!
• GitHub runs workflow jobs that use (unavailable) secrets. No warning, no error, no easy way to skip based on "is fork" or "has secrets". (I found a workaround.)
• WebDriver and SauceLabs happy start the test with empty string as username and access key and then timeout on random internal errors. No warning or error. Ref Uncaught TypeError: Cannot read property 'deleteSession' of null karma-runner/karma-sauce-launcher#236. This is where my failure started and it took a lot of digging for me to realize the previous points, which is that the secrets I had configured where in fact completely ignored and replaced by empty strings.
• Nightwatch and GitHub don't have built-in support for starting a SauceLabs connect tunnel, and WebDriver/SauceLabs don't warn against this, which resulted in additional confusing timeouts. I'd expect some kind of warning for "invalid tunnel-identifier" or "unknown host / tunnel needed" but no such thing. While the Karma/QUnit setup starts the tunnel automatically, for the End-to-end tests we start it in a separate step now. (Ref docs: Encourage use of v1 instead of master saucelabs/sauce-connect-action#22).

In conclusion:

• On forked pull requests (like this one) we run only the basic tests, the same as I had prepared on Travis previously.
• On pushes and local pull requests, we also run UI tests and unit tests via SauceLabs. These are gracefully skipped on PRs like this one. To verify that the secure tests for regular pushes pass as well, see https://github.com/kiwix/kiwix-js/actions/runs/451912893.

[mossroy]

Excellent!
Many thanks @Krinkle
One minor thing is about the wording of tests-basic and tests-secure : I fully understand the technical reasons to separate them like that, but it might be confusing names when one does not know this context. On might wonder : "are some CI tests secure, and some other ones not secure?"

[Jaifroid]

@Krinkle Looks excellent to me too. Thank you so much!

I noticed one unresolved conversation in the code: see here. Not sure if the comment is still relevant.

[mossroy]

As the remaining remarks are very minor, I propose to squash and merge this PR, if you agree @Jaifroid
So that to re-enable the unit and UI tests for other PRs

[Jaifroid]

@mossroy Yes of course!

[Jaifroid]

@Krinkle Thank you very much for this PR. A flag is appearing at the top of our Repo about a security vulnerability in a Node dependency, leading to this report:

I guess it's not a big deal (DoS attack), but any way you know of to update the dependency?

[Jaifroid]

Sorry for further quesiton @Krinkle, but after running npm install, then npm run test-unit-local, I see the following on my local machine (Windows 10):

Should Chrome headless be able to run this way? It doesn't seem to work for me. Do I need to take any extra steps?

[mossroy]

Regarding the ecstatic vulnerability, the right way to do that would be to upgrade http-server to a version that depends on a non-vulnerable version of ecstatic. This can easily be done in package.json file.
Unfortunately, version 0.12.3 is the latest one. The problem has been reported one month ago, but got no answer so far : http-party/http-server#666
It's probably not a real security issue in our context, so I'd suggest to wait for an upgrade of http-server to be available.

[mossroy]

Regarding the local execution of unit tests with npm, it works on my ubuntu 20.04 with Chromium (with CHROME_BIN=/usr/bin/chromium-browser npm run test-unit-local command line)
Maybe try to set CHROME_BIN environment variable to your local executable binary of Chrome (or Chromium, or maybe the new Edge)