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.
Details
Details
- Reviewers
anlambert - Group Reviewers
Reviewers - Maniphest Tasks
- T3122: Add tests for the Web Labels webpack plugin
npm test
Diff Detail
Diff Detail
- Repository
- rDWAPPS Web applications
- Branch
- weblabels-tests
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 4521 Build 6002: tox-on-jenkins Jenkins Build 6001: arc lint + arc unit
Event Timeline
Comment Actions
Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/185/ for more details.
Comment Actions
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.
Comment Actions
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
| package.json | ||
|---|---|---|
| 9 | Since the addition of cypress tests, we must ensure jest will not try to execute them. | |
Comment Actions
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?
Comment Actions
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).