Improve JavaScript code test coverage

[jelmer]

There is barely any test coverage for the javascript code at the moment. It would be great to at least have some basic tests to validate that the code is valid, and ideally also that it makes the right HTTP requests.

Edit: (addded by @ix5)

Related PRs

• [MAJOR] js: Move to webpack, pug from r.js, jade #800
• isso: js: Replace pug templates with plain js #822
• js: Add Jest testing #801
• Add puppeteer end-to-end integration test #807
• .github: Add Javascript action #826
• [js] [tests] Add unit tests for postbox, comment-loader, comment rendering; better stubbing/TZ handling #868
• [client] js: test/integration: Test API directly #884
• [client][ci] Tests: Add screenshot testing #889
• [CI] js: test: Decouple screenshots from regular e2e tests #890

[ix5]

If someone experienced in JS would like to propose a solution, go for it!

@blatinier you seemed to be interested in migrating (some) of the client code to VanillaJS or something similar - let's start by covering the existing code ;)

[ix5]

@stefangehn @vincentbernat @pellenilsson @rocka @Lucas-C pinging you for recommendations/insights into JS testing.

Baring any rewrite that gets rid of RequireJS/almond, the testing framework/solution/... should be compatible with these technologies. I'll throw jest into the room, not having any particular experience with it but it looks well-supported.

[Lucas-C]

I can confirm that, in my experience, jest is very good!

[ix5]

I can confirm that, in my experience, jest is very good!

That's great to hear! Do you have any particular examples of similar usage which might help in implementing testing for Isso?

In any case, we'd also have a need of bringing up the server part (via docker?) to test out responses (or find a way to mock them).

[Lucas-C]

Do you have any particular examples of similar usage which might help in implementing testing for Isso?

No I don't have any useful example at hand...

However, what would you think about also introducing a code linter as part of the GitHub Actions pipeline?
eslint for example. Maybe I could find the time to submit a PR introducing it...

[ix5]

No I don't have any useful example at hand...

No worries, maybe someone else has.

However, what would you think about also introducing a code linter as part of the GitHub Actions pipeline? eslint for example. Maybe I could find the time to submit a PR introducing it...

I'm not at all opposed to that. But same goes for linters as @jelmer and me wrote in #756 - adding automatic code "quality" analysis tools is nice and all, but that still means not one line of actual tests has been written.

I feel like every time we bring up this issue, the answer seems to be to add some "magic sauce" to this repo... Not a criticism of you, @Lucas-C, just an observation that the issue of missing tests is so broad and open-ended (many decisions to make, lots to write) that we tend to lose ourselves in easy fixes and digressions.

So my approach would be for us to settle on something, anything, even if it's imperfect, and begin writing tests to have some basic coverage. To do that, we'd need a way to specify what we want to have covered. Common user interactions, moderation actions, missing/malformed server responses, comparison of generated client HTML, ...

[Lucas-C]

I fully agree with you @ix5!

I had a quick look at the Javascript code, and DOM manipulation seems closely tied to isso functional logic 😔
It may be hard to test the code without an actual web browser.

I see several options:

• setup some JS unit tests (using Jest for example), but it may take quite some time and effort just to get 50% of code coverage
• setup a static analysis tool (a linter like ESLint), that is easy to add without any refactoring (even with JS code closely tied to DOM manipulation like here) and spot a large range of "coding mistakes", but that won't be as good a safeguard as tests to spot regressions
• setup integration tests, with a running isso backend & frontend, and test scenarios covering basic functionalities like comment posting (maybe using CucumberJS & WebDriverJS), which would be the best functional non-regression tests possible, as they would cover both the frontend & backend, but which are also the most complex to setup

Given the very reduced activity on isso over the past years, I admit that I personnaly don't feel motivated to invest much time in this task, but I'd be happy to discuss & review PRs to move forward in any of those direction 😊

[ix5]

@Lucas-C thank you for your insights!

I have managed to make some strides in reducing the interdependence and modernizing the codebase a bit. To have any chance of integrating Isso with an existing testing framework, I feel like this is a blocking necessity.

See ix5/isso@js-rework

I've dropped RequireJS and almond in favor of webpack (which seems to be the de facto standard nowadays).
Also, the hacks to use jade have been removed in favor of using pug.

[ix5]

Progress report

As of now, my branch has a complete port to CommonJS syntax instead of the previous RequireJS-style define(["foo/bar"], function(foobar) { ... }); style.

Jest is set up and I have a few simple tests configured, which can import some Isso modules and pass. Anything relying on document properties is a bit iffy to mock, but that works okay-ish. Will work on decoupling stuff from DOM.

[ix5]

@Lucas-C @stefangehn (and anyone else interested) feel free to contact me at the email address listed on my website (contact section), I'd love to talk to you about this and have a few questions.

Sadly, the IRC channel #isso (even on libera.chat) seems vacant.

[stefangehn]

@ix5 Wow, thanks for all the work. My attempt to replace jade with pug (without touching the other cruft) totally failed as I can read JS but otherwise have zero knowledge about the whole JS ecosystem and its rapid change from one technology to another.

I'm actually around on IRC, you can ping me (mETz) if you'd like to discuss the changes. I at least know a bit about testing in languages other than JS.

[ix5]

Also of note: https://github.com/jGleitz/isso-clientlib by @jGleitz

Would be great to somehow pull in a few tests of that hugely impressive effort. It's a shame that so many great initiatives live in outside or forked repos and seldomly manage to make their way into upstream Isso.

[ix5]

Pinging @Lucas-C and @stefangehn (and maybe @Kerumen) now that a working test suite for the client part is in place.

Would be really great if e.g. @Lucas-C could focus on the unit testing part and @Kerumen on the integration testing part?

I have finally managed to document how to set up and run the test suite(s), see:

• Testing (live at isso-comments.github.io/docs/technical-docs/testing)
• and Testing the client (live at isso-comments.github.io/docs/technical-docs/testing-client).

[BBaoVanC]

Just want to note this down here before I forget, it would be nice to look into using Codecov which can show test coverage on pull requests. It's a pretty popular bot which I've seen on many other open source projects.

[ix5]

Just want to note this down here before I forget, it would be nice to look into using Codecov which can show test coverage on pull requests. It's a pretty popular bot which I've seen on many other open source projects.

I'm also very partial to having coverage be more visible. However, adding a dependency on an external proprietary service still doesn't sit quite right with me.
I can see the diff stats presented to contributors being quite fancy and useful, though.

Points of reference:

• https://www.valentinog.com/blog/jest-coverage/
• https://hynek.me/articles/ditch-codecov-python/

Jest has --coverage and can be configured to fail if coverage falls below a certain percentage globally or lines per file. I already configured the python unit tests to fail below 70%, see https://github.com/posativ/isso/blob/668882f6c551c0cb979139ad62b2a97a0c968914/.github/workflows/python-tests.yml#L94-L97

[ix5]

I think coverage is at an acceptable level now. Closing.