Migrate unit tests to Mocha #5600
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jackfranklin
commented
Apr 7, 2020
jackfranklin
commented
Apr 7, 2020
jackfranklin
commented
Apr 7, 2020
jackfranklin
commented
Apr 7, 2020
|
I love the way the tests look, thanks for taking this on Jack! I can't wait to drop the custom test runner code. To answer your question, I'd prefer a single large PR in this case since the changes are mostly mechanical. |
jackfranklin
force-pushed
the
move-tests-to-mocha
branch
3 times, most recently
from
d58ef39
to
f465e2e
Compare
mathiasbynens
approved these changes
Apr 8, 2020
Rather than maintain our own test runner we should instead lean on the community and use Mocha which is very popular and also our test runner of choice in DevTools too. Note that this commit doesn't remove the TestRunner source as it's still used for other unit tests, but they will be updated in a future PR and then we can remove the TestRunner. The main bulk of this PR is updating the tests as the old TestRunner passed in contextual data via the `it` function callback whereas Mocha does not, so we introduce some helpers for the tests to make it easier.
Our custom TestRunner used to look for these but now we aren't using it let's enforce it using ESLint.
jackfranklin
force-pushed
the
move-tests-to-mocha
branch
from
4c1d055
to
d602cd9
Compare
Not all tests need a global browser + page, so we have helpers that can be imported on a per-test basis.
jackfranklin
force-pushed
the
move-tests-to-mocha
branch
from
a6f3a67
to
0d14c1a
Compare
| const GOLDEN_DIR = path.join(__dirname, 'golden-' + suffix); | ||
| const OUTPUT_DIR = path.join(__dirname, 'output-' + suffix); | ||
| if (fs.existsSync(OUTPUT_DIR)) | ||
| rm(OUTPUT_DIR); |
There was a problem hiding this comment.
Just hit this: rm() is undefined here.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@mathiasbynens this is a proof of concept of how we can move to Mocha by replacing the custom test runner with a few small helpers.
I have migrated cookies.spec.js and all the tests pass.
I'm keen to plough ahead and start migrating more test files, but wanted to raise this PR to check:
cookies.mocha.spec.jsrenaming we can run one test at a time rather than migrate in bulk. Would you rather one PR for one test file or I just do them all in bulk? The changes are mechanical mostly so I think one PR might be OK.