Page MenuHomeSoftware Heritage

Write tests for license parsing in the WebLabels plugin.
Changes PlannedPublic

Authored by vlorentz on Mar 1 2019, 2:11 PM.



I only wrote tests for a small part of the plugin to have your opinion on this.
If it's fine, I'll write more.

Test Plan

npm test

Diff Detail

rDWAPPS Web applications
No Linters Available
No Unit Test Coverage
Build Status
Buildable 4521
Build 6002: tox-on-jenkinsJenkins
Build 6001: arc lint + arc unit

Event Timeline

Thanks for bootstrapping tests on this!

It was also in my mind to add some but I wanted to have a minimal working example on
our frontend codebase before going further on the subject.

I intend to contact LibreJS authors next week to discuss about the approach and how the
current specification could be improved to handle the contemporary way of working with
JavaScript nowadays (aka bundling a lot of source code released under a free license).

As you noticed, it is not clear about how to handle multiple licenses and their combinations, this is pretty
rare but such cases exist and they should be handled.

I hope that this work may be released for everyone on the npm registry and moved into a dedicated repository.
Proceeding this way will allow to add proper tests on the plugin without polluting the swh-web repository.

Sorry, I totally forgot that diff. I just rebased it to master and apart a small modification to the jest invocation, tests sill pass so looks good to me.

From my point of view, it can be landed as is and further tests could be added later if we need to update or improve the webpack plugin.

I contacted LibreJS authors a couple of months ago but unfortunately I did not get any answers on the licenses combination corner cases we faced.
I have subscribed to the LibreJS mailing lists so I will check changelog on each release to see if there is some specification updates on these.

Just a nitpick before landing this, I enforce Javascript code style through the use of the eslint tool. I just added a yarn target (a09aa5fb24f7)
to run the tool and automatically fix what is easily fixable (indentation level, literal string style).
In order for eslint to support jest, you will also need to install the eslint-plugin-jest package with yarn and update the "plugins"
and "end" entries in the .eslinrc file of swh-web as explained here.
So once the .eslintrc file is modified, simply executing the following commands will be enough to fix the eslint warnings:

$ yarn add eslint-plugin-jest --dev
$ yarn eslint

Since the addition of cypress tests, we must ensure jest will not try to execute them.
Modifying the command to jest swh/web/assets/* seems sufficient to do so.

This revision is now accepted and ready to land.Aug 27 2019, 2:15 PM

I used jest because it was the first search result when I looked for test frameworks.

As we use mocha for the frontend tests, maybe it makes more sense to make these tests use mocha instead of jest?

As we use mocha for the frontend tests, maybe it makes more sense to make these tests use mocha instead of jest?

This will be better for consistency with the other written frontend tests indeed and reduce the number of dependencies to fetch with yarn.
Nevertheless, you will need to pick another assertion library (expect.js seems the simplest choice here
to port the existing code) and mocking library (sinon.js seems the most widely used with mocha).