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

run global setup before test files load; closes #4508 #4511

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

boneskull
Copy link
Contributor

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.

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

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 boneskull added type: feature enhancement proposal area: usability concerning user experience or interface semver-major implementation requires increase of "major" version number; "breaking changes" labels Nov 13, 2020
@boneskull boneskull self-assigned this Nov 13, 2020
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 94.138% when pulling 6bf7da7 on boneskull/issue/4508-global-fixtures into c6856ba on master.

@witrin
Copy link

witrin commented Nov 1, 2021

@boneskull Is there any chance of a merge in the near future? This issue is really a pain point for us.

@greguintow
Copy link

@juergba Could you take a look into this? It's still a pain

@mariobm
Copy link

mariobm commented Apr 26, 2022

What is the status of this one? It would be nice if it gets prioritized

@justinasfour04
Copy link

Can you please commit this

@justinasfour04
Copy link

@tj

@justinasfour04
Copy link

@boneskull

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a 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.

@JoshuaKGoldberg JoshuaKGoldberg added the status: blocked Waiting for something else to be resolved label Mar 4, 2024
await mocha.runGlobalTeardown(context);
}
done(...args);
});
Copy link
Member

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.

@mdrobny
Copy link

mdrobny commented Mar 27, 2024

So glad to see this PR being resurrected 🎉 this bug is a pain in the ass 😄
I understand that maintenance-crew need to take a look at this and approve/improve but if there is something I could help with to push this forward then I can try ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: usability concerning user experience or interface semver-major implementation requires increase of "major" version number; "breaking changes" status: blocked Waiting for something else to be resolved type: feature enhancement proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀 Feature: Global fixtures should run before any files are loaded
8 participants