Chore: add testing framework #2
Conversation
- Move linting from `test` to a test prerequisite, the `pretest` script.
- Add Mocha `test` and `test:watch` scripts:
- Tests are permitted in test/ and src/ per ticket requirements.
However, it's unclear why they'd be wanted in both. The test/
directory was also added to format and lint scripts.
- A throwaway test was added since the absence of all tests causes a
failure and to show that the setup works.
- Add Mocha, Sinon.JS, and Chokidar CLI to `devDependencies`:
- Mocha is used because that's what Services and Readers'
Infrastructure use and it's quite popular.
- Sinon.JS is used because MobileFrontend uses it and Readers'
Infrastructure is considering it (T151091*) and it's popular too.
- Chokidar CLI (chokidar-cli) was chosen because it's popular, quick,
and Chokidar is used by Live Server (live-server). watch and
npm-watch were each tried but appeared to function sporadically and
slowly on Ubuntu GNOME v17.04. `npm test -- -w` cannot be used
because 1) continuous linting is also wanted, 2) [new files are not
observed], 3) a maintainer indicated they were wanted to get rid of
watch functionality in the same report, and 4) separating watch and
testing responsibilities gives more flexibility in responding to
file changes. No attempts were made to filter or throttle files
observed.
*The [Services team template] and most of the [Readers' Infrastructure
repos] do not reference it.
[Services team template]: https://github.com/wikimedia/service-template-node/blob/3842da2/package.json
[Readers' Infrastructure repos]: as described on:
- https://www.mediawiki.org/wiki/Wikimedia_Reading_Infrastructure_team#Projects
- https://www.mediawiki.org/wiki/Reading/Component_responsibility
And discovered with:
$ find -not \( \( -name .git -o -name node_modules -o -name i18n -o -name sinonjs -o -name bundle -o -name .bundle \) -prune \) -type f|
xargs -rd\\n grep -El 'sinon|\.(calledOn|calledWith|spy)\('
mediawiki/tests/qunit/...
mediawiki/extensions/MobileFrontend/...
mediawiki/extensions/MultimediaViewer/...
mediawiki/extensions/TimedMediaHandler/...
[new files are not observed]: mochajs/mocha#2176
- A Mocha ESLint environment was deliberately omitted to force tests to
explicitly specify their dependencies even though the Mocha API is a
global.
Bug: T172432
package.json
Outdated
| "lint": "eslint '{src,test}/**/*.js'", | ||
| "pretest": "npm run lint", | ||
| "test": "mocha '{src,test}/**/*.test.js'", | ||
| "test:watch": "chokidar '**' --silent -c 'npm test'", |
There was a problem hiding this comment.
I'm not sure if you realized but I installed nodemon previously which wraps chokidar and some other niceties and does basically the same with a nice API. For example you can trigger restarts by typing rs in the watching process without having to touch a file.
I don't mind switching to raw chokidar but I strongly believe that we should be only using one file watching utility.
This command with nodemon I believe would be something like:
nodemon -w src -w test --exec "npm test"
Let me know what you think.
There was a problem hiding this comment.
Thanks for the tip! Initially filtered to src/ and test/ which means package, module, and other changes outside these directories will be undetected. File deletions do not appear to be observed.
test/server/templates/page.test.js
Outdated
| @@ -0,0 +1,10 @@ | |||
| const assert = require("assert"); | |||
| const mocha = require("mocha"); | |||
There was a problem hiding this comment.
Can you do here?:
const {describe, it} = require('mocha')Great idea to not include the globals IMO 👍
|
I pushed some commits on the other branch so this one should probably be rebased. Sorry for the pain |
Initially filtered to src/ and test/ which means package, module, and other changes outside these directories will be undetected. File deletions do not appear to be observed.
|
@joakin, not at all! I've merged master into this branch (as I was afraid we'd lost our comments). Please let me know if I've botched this or if you'd prefer I not stack PRs. I have a little more feedback on this patch that will probably have to come after my meetings which start in a few minutes. |
| "lint:all": "npm run lint -- '{src,test}/**/*.js'", | ||
| "pretest": "npm run lint:all", | ||
| "test": "mocha '{src,test}/**/*.test.js'", | ||
| "test:watch": "nodemon -w src -w test -x 'npm test'", | ||
| "precommit": "lint-staged" | ||
| }, | ||
| "lint-staged": { |
There was a problem hiding this comment.
Should this be npm run lint:all now?
There was a problem hiding this comment.
Do you think we should run the unit tests on precommit? We are doing it in popups and it hasn't been a problem since the unit tests have remained quite snappy. I'm fine just leaving npm test there.
There was a problem hiding this comment.
Now that I see, I may have misunderstood what lint-staged is for. Should we run lint-staged && npm test on precommit?
Nothing to do with this patch tho.
There was a problem hiding this comment.
I would like to run as many tests and checks as we can in the precommit hook. I wasn't sure if calling them from lint-staged was correct, however, since npm test are the unit tests (not linting) and npm run lint:all is executed as a prerequistite to tests (so it doesn't actually need to be called explicitly).
Move linting from
testto a test prerequisite, thepretestscript.Add Mocha
testandtest:watchscripts:However, it's unclear why they'd be wanted in both. The test/
directory was also added to format and lint scripts.
failure and to show that the setup works.
Add Mocha, Sinon.JS, and Chokidar CLI to
devDependencies:Infrastructure use and it's quite popular.
Infrastructure is considering it (T151091*) and it's popular too.
and Chokidar is used by Live Server (live-server). watch and
npm-watch were each tried but appeared to function sporadically and
slowly on Ubuntu GNOME v17.04.
npm test -- -wcannot be usedbecause 1) continuous linting is also wanted, 2) [new files are not
observed], 3) a maintainer indicated they were wanted to get rid of
watch functionality in the same report, and 4) separating watch and
testing responsibilities gives more flexibility in responding to
file changes. No attempts were made to filter or throttle files
observed.
*The Services team template and most of the [Readers' Infrastructure
repos] do not reference it.
[Readers' Infrastructure repos]: as described on:
And discovered with:
$ find -not ( ( -name .git -o -name node_modules -o -name i18n -o -name sinonjs -o -name bundle -o -name .bundle ) -prune ) -type f|
xargs -rd\n grep -El 'sinon|.(calledOn|calledWith|spy)('
mediawiki/tests/qunit/...
mediawiki/extensions/MobileFrontend/...
mediawiki/extensions/MultimediaViewer/...
mediawiki/extensions/TimedMediaHandler/...
[new files are not observed]: cli don't watch new added file mochajs/mocha#2176
A Mocha ESLint environment was deliberately omitted to force tests to
explicitly specify their dependencies even though the Mocha API is a
global.
Bug: T172432