-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
run global setup before test files load; closes #4508 #4511
base: main
Are you sure you want to change the base?
Conversation
BREAKING CHANGE `Mocha#run()` was orchestrating if and when to run global setup/teardown fixtures, but `Mocha#run()` must be called after test files have been loaded. This is a problem, because: 1. `--delay` may expect something created in a fixture to be accessible, but it won't be 2. Any future support for async suites is similarly negatively impacted 3. It was inconsistent between "watch" and "single run" mode; "watch" mode already has this behavior! This change causes setup fixtures to run _before_ any test files have been loaded. In Node.js, this happens in `lib/cli/run-helpers`. - Added two functions to `Mocha`; `Mocha#globalSetupEnabled()` and `Mocha#globalTeardownEnabled()` which both return booleans - Correct order of operations is asserted in `test/integration/global-fixtures.spec.js` - Removed low-value (and newly broken) unit tests where `Mocha#run` called `runGlobalSetup`/`runGlobalTeardown` - Add a note to `browser-entry.js` about global fixtures This is breaking because: 1. Browser users now must run setup/teardown fixtures manually. 2. Somebody's test harness might expect test files to be loaded (and suites run) _before_ global setup fixtures execute.
@boneskull Is there any chance of a merge in the near future? This issue is really a pain point for us. |
@juergba Could you take a look into this? It's still a pain |
What is the status of this one? It would be nice if it gets prioritized |
Can you please commit this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Shoutout to 2020-era @boneskull for writing clear, maintainable code. 😄
Since this is a semver-major
we'll definitely want reviews from the rest of @mochajs/maintenance-crew. Note that per #5027, even when we have approvals, we're still ramping up into bigger changes. This will take a while longer to get merged.
await mocha.runGlobalTeardown(context); | ||
} | ||
done(...args); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside: https://github.com/mochajs/mocha/pull/4771/files#r1511173111 also talks about refactoring these shared wrappers. If I were to end up taking ownership over this PR then I'd probably apply that refactor. Food for thought.
So glad to see this PR being resurrected 🎉 this bug is a pain in the ass 😄 |
BREAKING CHANGE
Mocha#run()
was orchestrating if and when to run global setup/teardown fixtures, butMocha#run()
must be called after test files have been loaded. This is a problem, because:--delay
may expect something created in a fixture to be accessible, but it won't beThis change causes setup fixtures to run before any test files have been loaded. In Node.js, this happens in
lib/cli/run-helpers
.Mocha
;Mocha#globalSetupEnabled()
andMocha#globalTeardownEnabled()
which both return booleanstest/integration/global-fixtures.spec.js
Mocha#run
calledrunGlobalSetup
/runGlobalTeardown
browser-entry.js
about global fixturesThis is breaking because:
We should probably merge this, even though I'm loathe to break it--the original design will cause major headaches in future development. My bad!
Closes #4508