Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get rid of "--delay" mocha CLI option #364

Merged
merged 8 commits into from Sep 22, 2021
Merged

Get rid of "--delay" mocha CLI option #364

merged 8 commits into from Sep 22, 2021

Conversation

tidoust
Copy link
Member

@tidoust tidoust commented Sep 22, 2021

This is needed to fix #363. The --delay option requires the code to call the run function only once and we were calling it multiple times. More importantly, Mocha does not catch exceptions thrown at the root level and simply reports nothing when that happens. These exceptions typically get thrown when the IDL cannot be parsed, meaning that invalid data exceptions mostly went unnoticed...

This update wraps all code that needs to run before tests in a before function. Note the need to keep a "dummy" test at the root level, otherwise the before function won't run (and wouldn't have anywhere to report failure). This workaround is somewhat documented in mocha's repository, see mochajs/mocha#2116 (comment)

This update slightly improves describe/it tests naming as well.

This is needed to fix #363. The `--delay` option requires the code to call the
`run` function only once and we were calling it multiple times. More
importantly, Mocha does not catch exceptions thrown at the root level and simply
reports nothing when that happens. These exceptions typically get thrown when
the IDL cannot be parsed, meaning that invalid data exceptions mostly went
unnoticed...

This update wraps all code that needs to run before tests in a `before`
function. Note the need to keep a "dummy" test at the root level, otherwise the
`before` function won't run (and wouldn't have anywhere to report failure).

This update slightly improves describe/it tests naming as well.
@tidoust
Copy link
Member Author

tidoust commented Sep 22, 2021

Dammit, I inadvertently committed additional unicity tests for CSS that cannot pass today. Will rollback...

Too early for that :)
@tidoust
Copy link
Member Author

tidoust commented Sep 22, 2021

Tests now fail because they actually run, and detect that startTime is defined twice. We'll need to fix that through patches. Anyway, that's orthogonal to this PR.

Copy link
Member

@dontcallmedom dontcallmedom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I fully understand the mocha magic, but I'm more than happy to trust that it works :)

A few nits worth fixing (even though they were not introduced by this PR)

test/idl/package.js Outdated Show resolved Hide resolved
test/css/all.js Outdated Show resolved Hide resolved
test/css/package.js Outdated Show resolved Hide resolved
test/elements/all.js Outdated Show resolved Hide resolved
test/css/parse.js Outdated Show resolved Hide resolved
test/elements/consistency.js Outdated Show resolved Hide resolved
tidoust and others added 6 commits September 22, 2021 17:59
Co-authored-by: Dominique Hazael-Massieux <dom@w3.org>
Co-authored-by: Dominique Hazael-Massieux <dom@w3.org>
Co-authored-by: Dominique Hazael-Massieux <dom@w3.org>
Co-authored-by: Dominique Hazael-Massieux <dom@w3.org>
Co-authored-by: Dominique Hazael-Massieux <dom@w3.org>
Co-authored-by: Dominique Hazael-Massieux <dom@w3.org>
@tidoust tidoust merged commit b3600e7 into master Sep 22, 2021
@tidoust tidoust deleted the tests-nodelay branch September 22, 2021 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running npm run test-idl does not run tests if IDL cannot be parsed
2 participants