Skip to content

Conversation

@boneskull
Copy link
Member

  • increase default timeout for wiggle room
  • specify watch-ignore in case we run our own tests in watch mode
  • reformat .mocharc.yml
  • integration/fixtures/uncaught/listeners.fixture.js: reduce number of listeners created to avoid max listener warning
  • integration/fixtures/options/watch.spec.js: do not use context object; be more specific about spawn options
  • integration/fixtures/diffs.spec.js: do not pass -C; the helper already does this
  • node-unit/mocha.spec.js: do not actually create files; it's a unit test after all.
  • node-unit/cli/config.spec.js: rewiremock fixes
  • node-unit/cli/options.spec.js: rewiremock fixes; increase timeout
  • unit/hook-timeout.spec.js: do not override default (configured) timeout
  • unit/runner.spec.js: leverage unexpected-eventemitter
  • unit/throw.spec.js: proper teardown: remove uncaught exception listeners
  • unit/timeout.spec.js: increase timeout to greater than default (configured) value

Ref: #4198

@boneskull boneskull added type: chore generally involving deps, tooling, configuration, etc. semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Apr 21, 2020
@boneskull boneskull self-assigned this Apr 21, 2020
@coveralls
Copy link

coveralls commented Apr 21, 2020

Coverage Status

Coverage increased (+0.002%) to 93.103% when pulling d5d0c98 on boneskull/issue/2839-test-fixes into 9c965c9 on master.

describe('when enabled', function() {
this.timeout(10 * 1000);
this.slow(3000);
let tempDir;
Copy link
Member Author

Choose a reason for hiding this comment

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

this is just me being fussy. but most all of our tests use function scope instead of the context to store data.

});

describe('loadConfig()', function() {
let parsers;
Copy link
Member Author

Choose a reason for hiding this comment

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

this "unit" test was reading files, and it shouldn't need to read files to assert what's trying to assert.

@boneskull
Copy link
Member Author

timeout-related changes were variously because of test flake, and because I implemented this while implementing parallel mode, the test execution times have a larger variance.

I'm starting to wonder if test timeouts aren't really that helpful, because I spend most of my time increasing the timeouts due to slow CI runs instead of catching significant performance issues.

boneskull added a commit that referenced this pull request Apr 24, 2020
the same code should be in PR #4240
boneskull added a commit that referenced this pull request Apr 24, 2020
the same code should be in PR #4240
boneskull added a commit that referenced this pull request Apr 24, 2020
the same code should be in PR #4240
@boneskull boneskull force-pushed the boneskull/issue/2839-test-fixes branch 3 times, most recently from 922b066 to 31ca866 Compare April 28, 2020 20:54
@boneskull boneskull requested a review from juergba April 28, 2020 20:55
describe('when supplied a filepath with ".js" extension', function() {
const filepath = 'foo.js';

it('should use the JS parser', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

A test for the .cjs extension is missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, didn't notice! just added one.

Copy link
Contributor

Choose a reason for hiding this comment

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

still missing

@boneskull boneskull force-pushed the boneskull/issue/2839-test-fixes branch from 31ca866 to 798155b Compare April 30, 2020 21:03
- increase default timeout for wiggle room
- specify `watch-ignore` in case we run our own tests in watch mode
- reformat `.mocharc.yml`
- `integration/fixtures/uncaught/listeners.fixture.js`: reduce number of listeners created to avoid max listener warning
- `integration/fixtures/diffs.spec.js`: do not pass `-C`; the helper already does this
- `integration/options/watch.spec.js`: do not use context object; be more specific about spawn options. tweak timings
- `node-unit/mocha.spec.js`: make more unit-test-like insofar as that's possible when testing w/ `require()`
- `node-unit/cli/config.spec.js`: rewiremock fixes; assertion tweaks; add test for `.cjs` extension
- `node-unit/cli/options.spec.js`: rewiremock fixes; increase timeout
- `unit/hook-timeout.spec.js`: do not override default (configured) timeout
- `unit/runner.spec.js`: leverage [unexpected-eventemitter](https://npm.im/unexpected-eventemitter)
- `unit/throw.spec.js`: proper teardown: remove uncaught exception listeners
- `unit/timeout.spec.js`: increase timeout to _greater than_ default (configured) value
- `example/config/.mocharc.yml`: quote yaml strings

BONUS

- fixes a weird call to `Mocha.unloadFile()` from `Mocha#unloadFiles()`

Ref: #4198
@boneskull boneskull requested a review from juergba April 30, 2020 21:19
@boneskull
Copy link
Member Author

@juergba I think this is ready for your approval again

Copy link
Contributor

@juergba juergba left a comment

Choose a reason for hiding this comment

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

The ".cjs" test is still missing.

describe('when supplied a filepath with ".js" extension', function() {
const filepath = 'foo.js';

it('should use the JS parser', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

still missing

@boneskull boneskull force-pushed the boneskull/issue/2839-test-fixes branch from 798155b to d5d0c98 Compare May 1, 2020 21:56
@boneskull
Copy link
Member Author

forgot to push, sorry. if this passes CI, I'll merge

@boneskull boneskull merged commit 2509ab5 into master May 4, 2020
@boneskull boneskull deleted the boneskull/issue/2839-test-fixes branch May 4, 2020 19:56
boneskull added a commit that referenced this pull request May 4, 2020
* test helper improvements

- enables `RawResult` (the result of `invokeMocha()` or `invokeMochaAsync()`) to check the exit code via `to have code` assertion
- add passed/failing/pending assertions for `RawRunResult` (the result of `runMocha()` or `runMochaAsync()`)

- expose `getSummary()`, which can be used with a `RawResult` (when not failing)
- reorganize the module a bit
- create `runMochaAsync()` and `runMochaJSONAsync()` which are like `runMocha()` and `runMochaJSON()` except return `Promise`s
- better trapping of JSON parse errors
- better default handling of `STDERR` output in subprocesses (print it instead of suppress it!)
- do not let the `DEBUG` env variable reach subprocesses _unless it was explicitly supplied_
- add an easily copy-paste-able `command` prop to summary
- add some missing docstrings

Ref: #4198

* increase timeout in watch test for CI

the same code should be in PR #4240
@craigtaub craigtaub added this to the next milestone May 21, 2020
craigtaub pushed a commit that referenced this pull request May 21, 2020
- increase default timeout for wiggle room
- specify `watch-ignore` in case we run our own tests in watch mode
- reformat `.mocharc.yml`
- `integration/fixtures/uncaught/listeners.fixture.js`: reduce number of listeners created to avoid max listener warning
- `integration/fixtures/diffs.spec.js`: do not pass `-C`; the helper already does this
- `integration/options/watch.spec.js`: do not use context object; be more specific about spawn options. tweak timings
- `node-unit/mocha.spec.js`: make more unit-test-like insofar as that's possible when testing w/ `require()`
- `node-unit/cli/config.spec.js`: rewiremock fixes; assertion tweaks; add test for `.cjs` extension
- `node-unit/cli/options.spec.js`: rewiremock fixes; increase timeout
- `unit/hook-timeout.spec.js`: do not override default (configured) timeout
- `unit/runner.spec.js`: leverage [unexpected-eventemitter](https://npm.im/unexpected-eventemitter)
- `unit/throw.spec.js`: proper teardown: remove uncaught exception listeners
- `unit/timeout.spec.js`: increase timeout to _greater than_ default (configured) value
- `example/config/.mocharc.yml`: quote yaml strings

BONUS

- fixes a weird call to `Mocha.unloadFile()` from `Mocha#unloadFiles()`

Ref: #4198
craigtaub pushed a commit that referenced this pull request May 21, 2020
* test helper improvements

- enables `RawResult` (the result of `invokeMocha()` or `invokeMochaAsync()`) to check the exit code via `to have code` assertion
- add passed/failing/pending assertions for `RawRunResult` (the result of `runMocha()` or `runMochaAsync()`)

- expose `getSummary()`, which can be used with a `RawResult` (when not failing)
- reorganize the module a bit
- create `runMochaAsync()` and `runMochaJSONAsync()` which are like `runMocha()` and `runMochaJSON()` except return `Promise`s
- better trapping of JSON parse errors
- better default handling of `STDERR` output in subprocesses (print it instead of suppress it!)
- do not let the `DEBUG` env variable reach subprocesses _unless it was explicitly supplied_
- add an easily copy-paste-able `command` prop to summary
- add some missing docstrings

Ref: #4198

* increase timeout in watch test for CI

the same code should be in PR #4240
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-patch implementation requires increase of "patch" version number; "bug fixes" type: chore generally involving deps, tooling, configuration, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants