Extend tests for `--watch` options #3915

geigerzaehler · 2019-05-14T16:12:10Z

Requirements

Test coverage for the --watch option needs to be improved. We also need a solid base to be able to test the changes proposed in #3912.

Description of the Change

This PR adds two test cases to test the --watch option. We check that touching a test file reruns the tests and we test that touching file that has a correct extensions reruns the test.

With the support code introduced we also update the one existing test.

The change uses promises instead of callbacks. For example we change runMochaJSONRaw to runMochaJSONRawAsync. The use of promises improves the control flow and gives better error messages.

Benefits

Better test coverage
It is now easy to write more watcher tests.
The use of promises provides better control flow guarantees and error messages.

Possible Drawbacks

The tests introduce fs-extra as a dev dependency. We need this for the following reasons:

Node v6 does not support fs.copyFile
Nodes vanilla fs module has no method to remove a directory and its content.

The tests require quite a lot of non-trivial support code.

With the introduction of runMochaJSONRawAsync we mix promise based and callback based APIs. While this is not ideal the benefits of using promises (described above) warrant this inconsistency

Applicable issues

#3912

jsf-clabot · 2019-05-14T16:12:15Z

All committers have signed the CLA.

coveralls · 2019-05-14T16:33:06Z

Coverage increased (+0.6%) to 92.414% when pulling c66e6a4 on geigerzaehler:add-integration-tests-for-file-watching into 750c5f8 on mochajs:master.

juergba · 2019-05-15T12:47:45Z

Yes, this is non-trivial stuff and I will need some time to digest this.

I have doubts about the change from callback to promises. I would have expexted the tests to crash, since our integration tests should also include browser and yes IE11 tests.
If you push your branch directly to our repos mochajs/mocha, then additional tests will run and your changes will probably fail. I'm not sure though...

runMochaJSONRaw: it has been required in just one single spec file?

geigerzaehler · 2019-05-15T16:02:39Z

I would have expexted the tests to crash, since our integration tests should also include browser and yes IE11 tests.

As far as I can see from karma.conf.js only the unit tests (test/unit/*.spec.js) are run in the browser. It seems to me that running the integration tests in the browser does not make sense since they actually consist of running the the mocha executable.

runMochaJSONRaw: it has been required in just one single spec file?

Yes, only in the watcher spec. (I would actually prefer to move it to the spec file but did not want to make to many changes.)

juergba · 2019-05-15T16:48:39Z

Thanks @geigerzaehler
I asked @boneskull to help me out and have a look at this.

juergba · 2019-05-16T07:56:50Z

I pushed your branch to this repo and both additional tests have succeeded.
Note to myself: don't worry, be happy.

boneskull · 2019-05-17T16:19:52Z

@juergba

I have doubts about the change from callback to promises. I would have expexted the tests to crash, since our integration tests should also include browser and yes IE11 tests.

This is untrue (unfortunately); only the stuff in test/unit and test/browser-specific run in a browser. I have an experimental branch where I'm trying to reconcile these and run some portion of the integration tests in a browser, but it's incomplete.

I was also hoping to switch to Promise-based test helpers, so this is a welcome change from my view.

boneskull

this is a welcome addition to our test suites, thank you!

I have a couple requested changes, IMO nothing major. please resolve them or discuss.

boneskull · 2019-05-17T16:22:06Z

.eslintrc.yml

@@ -25,6 +25,7 @@ overrides:
      - bin/*
      - lib/cli/**/*.js
      - test/node-unit/**/*.js
+      - test/integration/**/*.js


would prefer if we can list individual files here; would rather not open the floodgates to ES6 in the integration tests, as there are tentative plans to run some portion of them in a browser

boneskull · 2019-05-17T16:22:51Z

test/integration/helpers.js

@@ -282,7 +287,7 @@ function resolveFixturePath(fixture) {
  if (path.extname(fixture) !== '.js') {
    fixture += '.fixture.js';
  }
-  return path.join('test', 'integration', 'fixtures', fixture);
+  return path.resolve('test', 'integration', 'fixtures', fixture);


does this actually change anything?

This was necessary before but I changed the signature of runMochaJSONRawAsync to not use a special fixturePath argument. This allows us to revert this change and make the code more understandable.

3f6372e

test/integration/options/watch.spec.js

boneskull · 2019-05-17T16:24:19Z

test/integration/options/watch.spec.js

+ * Touch a file by appending a space to the end. Returns a promise that resolves
+ * when the file has been touched.
+ */
+function touchFile(file) {


this function doesn't return anything.

Updated the documentation in 3948056

test/integration/options/watch.spec.js

boneskull · 2019-05-17T16:25:23Z

test/integration/options/watch.spec.js

+  args = ['--watch'].concat(args);
+  const [mocha, mochaDone] = runMochaJSONRawAsync(fixture, args, spawnOpts);
+  const testResults = mochaDone.then(data => {
+    expect(data.code, 'to be', sigintExitCode);


I think we can skip this. these tests aren't asserting Mocha exits how we expect; they are --watch tests.

test/integration/helpers.js

package.json

geigerzaehler · 2019-05-19T09:00:44Z

Thanks for the review @boneskull. I hope I have addressed all your comments

test/integration/options/watch.spec.js

juergba · 2019-05-19T13:08:36Z

@boneskull why do we test with "cross-spawn", when in real life Mocha runs with node's "child_process"?

boneskull · 2019-05-19T15:29:54Z

I don’t think there’s a great reason. There are issues with child_process spawn on windows, but don’t know if those affect mocha in any meaningful way

juergba · 2019-05-19T15:55:34Z

@craigtaub could you review this PR, please? I think you have been the author of the first watch test.

juergba

@geigerzaehler thanks!

test/integration/helpers.js

craigtaub

Tiny comment. Nice work. LGTM

This commit adds two test cases to test the `--watch` option. We check that touching a test file reruns the tests and we test that touching a file that has a correct extensions reruns the test. This commit adds `fs-extra` as a new dev dependency.

boneskull

LGTM!

.eslintrc.yml

boneskull · 2019-05-21T16:53:46Z

@geigerzaehler thanks a ton!

geigerzaehler · 2019-05-22T10:30:55Z

@geigerzaehler thanks a ton!

Thanks for the support

geigerzaehler force-pushed the add-integration-tests-for-file-watching branch from bb66278 to 5a2124b Compare May 14, 2019 19:43

boneskull suggested changes May 17, 2019

View reviewed changes

juergba reviewed May 19, 2019

View reviewed changes

test/integration/options/watch.spec.js Outdated Show resolved Hide resolved

juergba requested a review from craigtaub May 19, 2019 15:53

juergba approved these changes May 20, 2019

View reviewed changes

craigtaub reviewed May 20, 2019

View reviewed changes

test/integration/helpers.js Outdated Show resolved Hide resolved

craigtaub approved these changes May 20, 2019

View reviewed changes

Extend tests for --watch options

c66e6a4

This commit adds two test cases to test the `--watch` option. We check that touching a test file reruns the tests and we test that touching a file that has a correct extensions reruns the test. This commit adds `fs-extra` as a new dev dependency.

geigerzaehler force-pushed the add-integration-tests-for-file-watching branch from 2777874 to c66e6a4 Compare May 21, 2019 08:46

boneskull approved these changes May 21, 2019

View reviewed changes

.eslintrc.yml Show resolved Hide resolved

boneskull merged commit a758154 into mochajs:master May 21, 2019

geigerzaehler deleted the add-integration-tests-for-file-watching branch May 22, 2019 10:30

juergba added this to the next milestone Jun 12, 2019

This was referenced Oct 8, 2019

[Snyk] Upgrade mocha from 6.2.0 to 6.2.1 Niweera/googledict#3

Open

[Snyk] Upgrade mocha from 6.2.0 to 6.2.1 Niweera/wordhound#6

Open

This was referenced Oct 23, 2019

[Snyk] Upgrade mocha from 6.2.0 to 6.2.1 Niweera/googledict#4

Open

[Snyk] Upgrade mocha from 4.1.0 to 6.2.1 ajesse11x/marketplace#5

Open

This was referenced Oct 26, 2019

[Snyk] Upgrade mocha from 2.5.3 to 6.2.1 ajesse11x/kompose#1

Open

[Snyk] Upgrade mocha from 2.5.3 to 6.2.2 ajesse11x/kompose#5

Open

[Snyk] Upgrade mocha from 6.2.0 to 6.2.2 Niweera/googledict#5

Open

snyk-bot mentioned this pull request Nov 25, 2019

[Snyk] Upgrade mocha from 2.4.5 to 6.2.2 ajesse11x/cypress#12

Open

snyk-bot mentioned this pull request Apr 9, 2020

[Snyk] Upgrade mocha from 6.0.2 to 6.2.2 civilizador/ticket-master#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend tests for `--watch` options #3915

Extend tests for `--watch` options #3915

geigerzaehler commented May 14, 2019

jsf-clabot commented May 14, 2019 •

edited

coveralls commented May 14, 2019 •

edited

juergba commented May 15, 2019

geigerzaehler commented May 15, 2019

juergba commented May 15, 2019

juergba commented May 16, 2019

boneskull commented May 17, 2019

boneskull left a comment

boneskull May 17, 2019

geigerzaehler May 19, 2019

boneskull May 17, 2019

geigerzaehler May 19, 2019

boneskull May 17, 2019

geigerzaehler May 19, 2019

boneskull May 17, 2019

geigerzaehler May 19, 2019

geigerzaehler commented May 19, 2019

juergba commented May 19, 2019

boneskull commented May 19, 2019

juergba commented May 19, 2019

juergba left a comment

craigtaub left a comment •

edited

boneskull left a comment

boneskull commented May 21, 2019

geigerzaehler commented May 22, 2019

Extend tests for --watch options #3915

Extend tests for --watch options #3915

Conversation

geigerzaehler commented May 14, 2019

Requirements

Description of the Change

Benefits

Possible Drawbacks

Applicable issues

jsf-clabot commented May 14, 2019 • edited

coveralls commented May 14, 2019 • edited

juergba commented May 15, 2019

geigerzaehler commented May 15, 2019

juergba commented May 15, 2019

juergba commented May 16, 2019

boneskull commented May 17, 2019

boneskull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geigerzaehler commented May 19, 2019

juergba commented May 19, 2019

boneskull commented May 19, 2019

juergba commented May 19, 2019

juergba left a comment

Choose a reason for hiding this comment

craigtaub left a comment • edited

Choose a reason for hiding this comment

boneskull left a comment

Choose a reason for hiding this comment

boneskull commented May 21, 2019

geigerzaehler commented May 22, 2019

Extend tests for `--watch` options #3915

Extend tests for `--watch` options #3915

jsf-clabot commented May 14, 2019 •

edited

coveralls commented May 14, 2019 •

edited

craigtaub left a comment •

edited