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.

Sign up for GitHub

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

Jump to bottom

Clean up internal test/ directory structure #1511

Closed
Krinkle opened this issue Nov 17, 2020 · 2 comments
Closed

Clean up internal test/ directory structure #1511

Krinkle opened this issue Nov 17, 2020 · 2 comments
Assignees
@Krinkle
Labels
Category: Tests help welcome Type: Enhancement New idea or feature request.

Comments

@Krinkle
Copy link
Member

Krinkle commented Nov 17, 2020 •
edited
Loading

We currently have many ways we run unit tests:

  1. In a browser (via Headless Chromium, grunt-contrib-qunit).
  2. In a browser with a Web Worker (via Headless Chromium, grunt-contrib-qunit).
  3. In Node.js, plainly (via a custom Grunt task).
  4. In Node.js using the QUnit CLI.
  5. In SpiderMonkey (via mozjs).

Outcome for this issue:

  • To the extent possible, have the same these three test runners automatically share a common base list of test suites. Or, have build script that can keep them in sync. Or, have a small test that just tells us whether they are in sync and add that to npm test.
  • In the repository, organise tests such that it is easy to spot which tests are common, which specific to browser, and which specific to CLI. Perhaps by giving those three use cases their own subdirectory, or by a mark in file name, or file comment.

For later:

  • Walk through the tests that don't run in all three and determine whether there is a good reason for it, and document that reason. And if there isn't one, have it run in all three. From a quick check it seems that a big chunk of tests only ever run in the browser and in plain Node. The CLI test run seems limited to only CLI-specific stuff mostly. It'd be good to have the bulk of the tests run through there as well.
Krinkle added a commit that referenced this issue Nov 17, 2020
@Krinkle
CLI: Add native support for ESM .mjs files on Node 12+
4f3ce8d 
* Rename test/es2017 and update ESLint config to allow for import/export
  syntax to be used. We don't rely on ESLint to know what is supported
  in Node.js, our test matrix in CI covers that already.

  We should probably re-think the way these tests are organized,
  which I've filed #1511 for.

* Update eslint config for src/cli to es2020. The dynamic `import()`
  statement was only spe'ced in es2020. This is first implemented
  (without experimental flag) in Node 12. We need to support Node 10.
  However it looks like Node 10 already tolerated this (maybe it sees
  it as a normal function, or maybe it was already recognised by the
  V8 parser it shipped with), so the try-catch suffices there.
  Again, the CI test matrix verifies for us that stuff works fine
  on older versions.

Ref #1465.
Krinkle added a commit that referenced this issue Nov 17, 2020
@Krinkle
CLI: Add native support for ESM .mjs files on Node 12+
d7beaf1 
* Rename test/es2017 and update ESLint config to allow for import/export
  syntax to be used. We don't rely on ESLint to know what is supported
  in Node.js, our test matrix in CI covers that already.

  We should probably re-think the way these tests are organized,
  which I've filed #1511 for.

* Update eslint config for src/cli to es2020. The dynamic `import()`
  statement was only spe'ced in es2020. This is first implemented
  (without experimental flag) in Node 12. We need to support Node 10.
  However it looks like Node 10 already tolerated this (maybe it sees
  it as a normal function, or maybe it was already recognised by the
  V8 parser it shipped with), so the try-catch suffices there.
  Again, the CI test matrix verifies for us that stuff works fine
  on older versions.

Ref #1465.
Krinkle added a commit that referenced this issue Nov 18, 2020
@Krinkle
CLI: Add native support for ESM .mjs files on Node 12+
b988b0b 
* Rename test/es2017 and update ESLint config to allow for import/export
  syntax to be used. We don't rely on ESLint to know what is supported
  in Node.js, our test matrix in CI covers that already.

  We should probably re-think the way these tests are organized,
  which I've filed #1511 for.

* Update eslint config for src/cli to es2020. The dynamic `import()`
  statement was only spe'ced in es2020. This is first implemented
  (without experimental flag) in Node 12. We need to support Node 10.
  However it looks like Node 10 already tolerated this (maybe it sees
  it as a normal function, or maybe it was already recognised by the
  V8 parser it shipped with), so the try-catch suffices there.
  Again, the CI test matrix verifies for us that stuff works fine
  on older versions.

Ref #1465.
Krinkle added a commit that referenced this issue Nov 18, 2020
@Krinkle
CLI: Add native support for ESM .mjs files on Node 12+
38a534c 
* Rename test/es2017 and update ESLint config to allow for import/export
  syntax to be used. We don't rely on ESLint to know what is supported
  in Node.js, our test matrix in CI covers that already.

  We should probably re-think the way these tests are organized,
  which I've filed #1511 for.

* Update eslint config for src/cli to es2020. The dynamic `import()`
  statement was only spe'ced in es2020. This is first implemented
  (without experimental flag) in Node 12. We need to support Node 10.
  However it looks like Node 10 already tolerated this (maybe it sees
  it as a normal function, or maybe it was already recognised by the
  V8 parser it shipped with), so the try-catch suffices there.
  Again, the CI test matrix verifies for us that stuff works fine
  on older versions.

Ref #1465.
Krinkle added a commit that referenced this issue Nov 18, 2020
@Krinkle
CLI: Add native support for ESM .mjs files on Node 12+
fdd01bc 
* Rename test/es2017 and update ESLint config to allow for import/export
  syntax to be used. We don't rely on ESLint to know what is supported
  in Node.js, our test matrix in CI covers that already.

  We should probably re-think the way these tests are organized,
  which I've filed #1511 for.

* Update eslint config for src/cli to es2020. The dynamic `import()`
  statement was only spe'ced in es2020. This is first implemented
  (without experimental flag) in Node 12. We need to support Node 10.
  However it looks like Node 10 already tolerated this (maybe it sees
  it as a normal function, or maybe it was already recognised by the
  V8 parser it shipped with), so the try-catch suffices there.
  Again, the CI test matrix verifies for us that stuff works fine
  on older versions.

Ref #1465.
Krinkle added a commit that referenced this issue Nov 18, 2020
@Krinkle
CLI: Add native support for ESM .mjs files on Node 12+
7369f47 
* Rename test/es2017 and update ESLint config to allow for import/export
  syntax to be used. We don't rely on ESLint to know what is supported
  in Node.js, our test matrix in CI covers that already.

  We should probably re-think the way these tests are organized,
  which I've filed #1511 for.

* Update eslint config for src/cli to es2020. The dynamic `import()`
  statement was only spe'ced in es2020. This is first implemented
  (without experimental flag) in Node 12. We need to support Node 10.
  However it looks like Node 10 already tolerated this (maybe it sees
  it as a normal function, or maybe it was already recognised by the
  V8 parser it shipped with), so the try-catch suffices there.
  Again, the CI test matrix verifies for us that stuff works fine
  on older versions.

Ref #1465.
Krinkle added a commit that referenced this issue Nov 20, 2020
@Krinkle
Build: Rewrite test-on-node task to use QUnit.on()
621031f 
Ref #1511.
@Krinkle Krinkle mentioned this issue Nov 20, 2020
Merged
Krinkle added a commit that referenced this issue Nov 20, 2020
@Krinkle
Build: Rewrite test-on-node task to use QUnit.on()
7f408b7 
Ref #1511.
Krinkle added a commit that referenced this issue Nov 20, 2020
@Krinkle
Build: Rewrite test-on-node task to use QUnit.on()
cb0c8f5 
Ref #1511.
Krinkle added a commit that referenced this issue Nov 22, 2020
@Krinkle
Build: Rewrite test-on-node task to use QUnit.on()
6b50747 
Ref #1511.
@Krinkle
Copy link
Member Author

Krinkle commented Nov 22, 2020

From #1516:

test-on-node is still the first task, […]. Once we've added the "main" tests to the CLI runs […], we may want to re-arrange it so that the CLI and browser test run first.

Krinkle added a commit that referenced this issue Nov 23, 2020
@Krinkle
Tests: Clean up hierarchy of our own test modules
a1bf287 
Our internal tests mostly predate support for nested modules
so that's why these are mostly flat and messy.
Re-structure them so that related tests are under the same
module. This makes it easier to scan the results and to re-run
tests in a given file.

Also:
* Use assert.test instead of QUnit.config.current where possible.
* Remove redundant use of assert.expect() from most tests.
* Remove some duplicate tests.
* Clarify local name in setTimeout.js as globalThis instead of
  window, since the test is used in Node.js as well.

Ref #1511.
Krinkle added a commit that referenced this issue Nov 23, 2020
@Krinkle
Tests: Clean up hierarchy of our own test modules
0ab79bd 
Our internal tests mostly predate support for nested modules
so that's why these are mostly flat and messy.
Re-structure them so that related tests are under the same
module. This makes it easier to scan the results and to re-run
tests in a given file.

Also:
* Use assert.test instead of QUnit.config.current where possible.
* Remove redundant use of assert.expect() from most tests.
* Remove some duplicate tests.
* Clarify local name in setTimeout.js as globalThis instead of
  window, since the test is used in Node.js as well.

Ref #1511.
Krinkle added a commit that referenced this issue Nov 23, 2020
@Krinkle
Tests: Remove redundant setTimeout.html file
8b9b275 
The test suite in this HTML file is already covered by the
main test/index.html file, as well as by the test-on-node task.

Ref #1511.
Krinkle added a commit that referenced this issue Nov 28, 2020
@Krinkle
CLI: Add native support for ESM .mjs files on Node 12+
74c8855 
* Rename test/es2017 and update ESLint config to allow for import/export
  syntax to be used. We don't rely on ESLint to know what is supported
  in Node.js, our test matrix in CI covers that already.

  We should probably re-think the way these tests are organized,
  which I've filed #1511 for.

* Update eslint config for src/cli to es2020. The dynamic `import()`
  statement was only spe'ced in es2020. This is first implemented
  (without experimental flag) in Node 12. We need to support Node 10.
  However it looks like Node 10 already tolerated this (maybe it sees
  it as a normal function, or maybe it was already recognised by the
  V8 parser it shipped with), so the try-catch suffices there.
  Again, the CI test matrix verifies for us that stuff works fine
  on older versions.

Ref #1465.
Closes #1510.
Krinkle added a commit that referenced this issue Jun 5, 2021
@Krinkle
Tests: Run each.js tests in test-on-node and mozjs.js too
b211ffb 
Ref #1569.
Ref #1511.
@Krinkle Krinkle mentioned this issue Jun 5, 2021
Merged
Krinkle added a commit that referenced this issue Jun 5, 2021
@Krinkle
Tests: Run each.js tests in test-on-node and mozjs.js too
47576c1 
Ref #1569.
Ref #1511.
Krinkle added a commit that referenced this issue Jun 6, 2021
@Krinkle
Tests: Enable test/main/promise.js in mozjs CI job
06e1135 
It just had one inline call to setTimeout in one of the test cases,
which is easily changed to use our local `defer` function instead.
After that, it passed.

While at it, simplify and synchronise the two mock-promise functions.
Not yet worth abstracting I think, but thinking about it.

Ref #1511.
Krinkle added a commit that referenced this issue Jun 6, 2021
@Krinkle
Tests: Enable test/main/promise.js in mozjs CI job
794e632 
It just had one inline call to setTimeout in one of the test cases,
which is easily changed to use our local `defer` function instead.
After that, it passed.

While at it, simplify and synchronise the two mock-promise functions.
Not yet worth abstracting I think, but thinking about it.

Ref #1511.
Krinkle added a commit that referenced this issue Jun 7, 2021
@Krinkle
Tests: Enable test/main/promise.js in mozjs CI job
bc7c287 
It just had one inline call to setTimeout in one of the test cases,
which is easily changed to use our local `defer` function instead.
After that, it passed.

While at it, simplify and synchronise the two mock-promise functions.
Not yet worth abstracting I think, but thinking about it.

Ref #1511.
Krinkle added a commit that referenced this issue Jun 7, 2021
@Krinkle
Tests: Enable test/main/promise.js in mozjs CI job
bea9feb 
It just had one inline call to setTimeout in one of the test cases,
which is easily changed to use our local `defer` function instead.
After that, it passed.

While at it, simplify and synchronise the two mock-promise functions.
Not yet worth abstracting I think, but thinking about it.

Ref #1511.
Krinkle added a commit to Krinkle/qunit that referenced this issue Jun 28, 2021
@Krinkle
Test: Change tests to not rely on "double done" hack
ceec61b 
The next commit in this branch for qunitjs#1511, will disallow adding
tests if `QUnit.done()` and `runEnd` have already happened, thus
leading these hacks to fail as follows:

````
Running "qunit:all" (qunit) task
Testing http://localhost:4000/test/index.html
[…]
Testing http://localhost:4000/test/module-skip.html ....
Error: Unexpected new test after the run already ended
    at new Test (http://localhost:4000/qunit/qunit.js:2206:13)
^C
```

In addition, due to a known issue in grunt-contrib-qunit, these would
also indefinitely hack instead of actually failing.

Ref gruntjs/grunt-contrib-qunit#178.
Ref qunitjs#1377.
Ref qunitjs#1511.
Krinkle added a commit that referenced this issue Jul 3, 2021
@Krinkle
Test: Change tests to not rely on "double done" hack
78bda44 
The next commit in this branch for #1511, will disallow adding
tests if `QUnit.done()` and `runEnd` have already happened, thus
leading these hacks to fail as follows:

````
Running "qunit:all" (qunit) task
Testing http://localhost:4000/test/index.html
[…]
Testing http://localhost:4000/test/module-skip.html ....
Error: Unexpected new test after the run already ended
    at new Test (http://localhost:4000/qunit/qunit.js:2206:13)
^C
```

In addition, due to a known issue in grunt-contrib-qunit, these would
also indefinitely hack instead of actually failing.

Ref gruntjs/grunt-contrib-qunit#178.
Ref #1377.
Ref #1511.
@Krinkle Krinkle mentioned this issue Jul 5, 2021
Merged
@Krinkle
Copy link
Member Author

Krinkle commented Jul 5, 2021

qunit/Gruntfile.js

Lines 139 to 151 in 66081d6

// FIXME: These tests use an ugly hack that re-opens
// an already finished test run. This only works reliably
// via the HTML Reporter thanks to some delays in the bridge.
// These tests are about reporting, not about functional
// behaviour. They would be best run either as reflection on the
// DOM in an HTML Reporter test, or from the CLI by asserting
// TAP output. I suggest we do the latter, and then remove them
// from here.
//
// Ref https://github.com/qunitjs/qunit/issues/1511
//
// "test/module-skip.js",
// "test/module-todo.js",

As of #1629, the module-skip.js and module-todo.js test suites might pass on plain node.

Krinkle added a commit to Krinkle/qunit that referenced this issue Aug 8, 2021
@Krinkle
Tests: Replace onerror/inside-test.html with a main test
7315b50 
* This additional entry point is no longer needed now.
  It was already redundant with test/index.html.

* Rename the file to match the `QUnit.module()` call.

* Include it in the test run for mozjs
  (previously it was only in index.html and test-on-node).

Ref qunitjs#1511.
Krinkle added a commit that referenced this issue Aug 9, 2021
@Krinkle
Tests: Move only-each.html JS to the right place
b134792 
The file was mixed in the main/ directory, but not actually
used by any of the main test runs (index.html, test-on-node, mozjs).

Ref #1511.
Krinkle added a commit that referenced this issue Aug 9, 2021
@Krinkle
Tests: Enable all test suites in CI
e453258 
Several of the HTML test suites were previously omitted from CI,
as they either couldn't pass on PhantomJS, or were simply forgotten.

* Add test/headless.html. Also updated to use `QUnit.on()`, and to hide
  debug messages when run in CI to avoid adding this noise.
* Add test/module-filter.html
* Add test/performance-mark.html
* Add test/webWorker.html. Ref #1171.
* Remove test/each.html. Duplicate of test/index.html.
* Remove test/stack-errors.html. Redundant with other tests for "noglobals"
  and "onUncaughtError" outside test context etc.
* Improve test/sandboxed-iframe.html, which was passing CI
  but failing manually, due to undefined `__grunt_contrib_qunit__`. We
  now define a fallback locally to ease debugging.

Added to test-on-node: test/module-skip.js, test/module-todo.js. These
were disabled due to use of hacky `QUnit.done()`, but that was recently
fixed with 78bda44.

Also add various main tests to test/webWorker.js, which had not been
updated in a while. The next commit will add a meta CLI test that
ensures that these are all in sync to avoid staleness in the future.

Ref #1511.
Krinkle added a commit that referenced this issue Aug 9, 2021
@Krinkle
Tests: Add structure test to keep suites in sync
32bce94 
Bump cli/eslintrc from es2017 to es2018 to allow use of the RegExp `s`
flag (dotAll). This is supported since Node 8+.

Ref #1511.
Krinkle added a commit to Krinkle/qunit that referenced this issue Aug 9, 2021
@Krinkle
Tests: Enable all test suites in CI
e80994d 
Several of the HTML test suites were previously omitted from CI,
as they either couldn't pass on PhantomJS, or were simply forgotten.

* Add test/headless.html. Also updated to use `QUnit.on()`, and to hide
  debug messages when run in CI to avoid adding this noise.
* Add test/module-filter.html
* Add test/performance-mark.html. Ref qunitjs#1319.
* Add test/webWorker.html. Ref qunitjs#1171.
* Remove test/each.html. Duplicate of test/index.html.
* Remove test/stack-errors.html. Redundant with other tests for "noglobals"
  and "onUncaughtError" outside test context etc.
* Improve test/sandboxed-iframe.html, which was passing CI
  but failing manually, due to undefined `__grunt_contrib_qunit__`. We
  now define a fallback locally to ease debugging.

Added to test-on-node: test/module-skip.js, test/module-todo.js. These
were disabled due to use of hacky `QUnit.done()`, but that was recently
fixed with 78bda44.

Also add various main tests to test/webWorker.js, which had not been
updated in a while. The next commit will add a meta CLI test that
ensures that these are all in sync to avoid staleness in the future.

Ref qunitjs#1511.
Krinkle added a commit to Krinkle/qunit that referenced this issue Aug 9, 2021
@Krinkle
Tests: Add structure test to keep suites in sync
fc9d447 
Bump cli/eslintrc from es2017 to es2018 to allow use of the RegExp `s`
flag (dotAll). This is supported since Node 8+.

Ref qunitjs#1511.
@Krinkle Krinkle mentioned this issue Aug 9, 2021
Merged
@Krinkle Krinkle self-assigned this Aug 9, 2021
Krinkle added a commit to Krinkle/qunit that referenced this issue Aug 9, 2021
@Krinkle
Tests: Enable all test suites in CI
39582f9 
Several of the HTML test suites were previously omitted from CI,
as they either couldn't pass on PhantomJS, or were simply forgotten.

* Add test/headless.html. Also updated to use `QUnit.on()`, and to hide
  debug messages when run in CI to avoid adding this noise.
* Add test/module-filter.html
* Add test/performance-mark.html. Ref qunitjs#1319.
* Add test/webWorker.html. Ref qunitjs#1171.
* Remove test/each.html. Duplicate of test/index.html.
* Remove test/stack-errors.html. Redundant with other tests for "noglobals"
  and "onUncaughtError" outside test context etc.
* Improve test/sandboxed-iframe.html, which was passing CI
  but failing manually, due to undefined `__grunt_contrib_qunit__`. We
  now define a fallback locally to ease debugging.

Added to test-on-node: test/module-skip.js, test/module-todo.js. These
were disabled due to use of hacky `QUnit.done()`, but that was recently
fixed with 78bda44.

Also add various main tests to test/webWorker.js, which had not been
updated in a while. The next commit will add a meta CLI test that
ensures that these are all in sync to avoid staleness in the future.

Ref qunitjs#1511.
Krinkle added a commit to Krinkle/qunit that referenced this issue Aug 9, 2021
@Krinkle
Tests: Add structure test to keep suites in sync
f60adc3 
Bump cli/eslintrc from es2017 to es2018 to allow use of the RegExp `s`
flag (dotAll). This is supported since Node 8+.

Ref qunitjs#1511.
Krinkle added a commit that referenced this issue Aug 9, 2021
@Krinkle
Tests: Replace onerror/inside-test.html with a main test
9e392ed 
* This additional entry point is no longer needed now.
  It was already redundant with test/index.html.

* Rename the file to match the `QUnit.module()` call.

* Include it in the test run for mozjs
  (previously it was only in index.html and test-on-node).

Ref #1511.
Krinkle added a commit that referenced this issue Aug 9, 2021
@Krinkle
Tests: Move only-each.html JS to the right place
3128947 
The file was mixed in the main/ directory, but not actually
used by any of the main test runs (index.html, test-on-node, mozjs).

Ref #1511.
Krinkle added a commit that referenced this issue Aug 9, 2021
@Krinkle
Tests: Enable all test suites in CI
c43785d 
Several of the HTML test suites were previously omitted from CI,
as they either couldn't pass on PhantomJS, or were simply forgotten.

* Add test/headless.html. Also updated to use `QUnit.on()`, and to hide
  debug messages when run in CI to avoid adding this noise.
* Add test/module-filter.html
* Add test/performance-mark.html. Ref #1319.
* Add test/webWorker.html. Ref #1171.
* Remove test/each.html. Duplicate of test/index.html.
* Remove test/stack-errors.html. Redundant with other tests for "noglobals"
  and "onUncaughtError" outside test context etc.
* Improve test/sandboxed-iframe.html, which was passing CI
  but failing manually, due to undefined `__grunt_contrib_qunit__`. We
  now define a fallback locally to ease debugging.

Added to test-on-node: test/module-skip.js, test/module-todo.js. These
were disabled due to use of hacky `QUnit.done()`, but that was recently
fixed with 78bda44.

Also add various main tests to test/webWorker.js, which had not been
updated in a while. The next commit will add a meta CLI test that
ensures that these are all in sync to avoid staleness in the future.

Ref #1511.
Krinkle added a commit that referenced this issue Aug 9, 2021
@Krinkle
Tests: Add structure test to keep suites in sync
159f932 
Bump cli/eslintrc from es2017 to es2018 to allow use of the RegExp `s`
flag (dotAll). This is supported since Node 8+.

Ref #1511.
@Krinkle Krinkle closed this as completed in c2a6a0f Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Krinkle Krinkle

Labels
Category: Tests help welcome Type: Enhancement New idea or feature request.
Milestone
No milestone
Development

No branches or pull requests
1 participant
@Krinkle