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

Add option testSequencer allow user use custom sequencer. #8223

Merged
merged 30 commits into from
Apr 2, 2019

Conversation

WeiAnAn
Copy link
Contributor

@WeiAnAn WeiAnAn commented Mar 26, 2019

Summary

This PR implement feature mention in #6194.
After some discussion, change to move testSequencer into individual package.
Give an option like runner, let user can user custom sequencer to handle tests order.

Test plan

  • update unit tests
  • add new e2e test

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great! Left some inline comments, but overall this looks awesome.

Mind updating the changelog in addition to addressing inline comments?

packages/jest-core/src/runJest.ts Outdated Show resolved Hide resolved
e2e/custom-test-sequencer/testSequencer.js Outdated Show resolved Hide resolved
packages/jest-cli/src/cli/args.ts Outdated Show resolved Hide resolved
packages/jest-test-sequencer/src/index.ts Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Mar 27, 2019

Codecov Report

Merging #8223 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8223      +/-   ##
==========================================
+ Coverage   62.27%   62.35%   +0.08%     
==========================================
  Files         265      265              
  Lines       10573    10560      -13     
  Branches     2572     2566       -6     
==========================================
+ Hits         6584     6585       +1     
+ Misses       3399     3387      -12     
+ Partials      590      588       -2
Impacted Files Coverage Δ
packages/jest-config/src/Defaults.ts 100% <ø> (ø) ⬆️
packages/jest-test-sequencer/src/index.ts 89.36% <ø> (ø)
packages/jest-config/src/index.ts 12.5% <ø> (ø) ⬆️
packages/jest-config/src/ValidConfig.ts 100% <ø> (ø) ⬆️
packages/jest-core/src/runJest.ts 34.66% <100%> (+0.88%) ⬆️
packages/jest-config/src/utils.ts 75.86% <100%> (+0.86%) ⬆️
packages/jest-config/src/normalize.ts 77.99% <100%> (+0.28%) ⬆️
packages/jest-core/src/ReporterDispatcher.ts 81.25% <0%> (-2.97%) ⬇️
packages/jest-runtime/src/index.ts 69.16% <0%> (-0.23%) ⬇️
packages/babel-plugin-jest-hoist/src/index.ts 0% <0%> (ø) ⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f246058...745cad3. Read the comment docs.

@WeiAnAn
Copy link
Contributor Author

WeiAnAn commented Mar 27, 2019

Should we provide a package/api like create-jest-ruuner.
It will make writing custom sequencer easier.

e2e/custom-test-sequencer/testSequencer.js Outdated Show resolved Hide resolved
e2e/custom-test-sequencer/testSequencer.js Show resolved Hide resolved
packages/jest-cli/src/cli/args.ts Show resolved Hide resolved
packages/jest-config/src/normalize.ts Show resolved Hide resolved
Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

What @SimenB said, plus a bunch of nits :)

docs/Configuration.md Outdated Show resolved Hide resolved
docs/Configuration.md Outdated Show resolved Hide resolved
docs/Configuration.md Outdated Show resolved Hide resolved
docs/Configuration.md Show resolved Hide resolved
Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

Thanks a lot @WeiAnAn! I left a few nits.

CHANGELOG.md Outdated Show resolved Hide resolved
docs/CLI.md Outdated Show resolved Hide resolved
docs/Configuration.md Outdated Show resolved Hide resolved
packages/jest-config/src/normalize.ts Show resolved Hide resolved
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Thank you for the great work here!

We've released 24.6, so could you merge in master, and ensure version numbers are correct and that the changelog entries are in the correct place? Happy to merge after that!

packages/jest-core/package.json Outdated Show resolved Hide resolved
@WeiAnAn
Copy link
Contributor Author

WeiAnAn commented Apr 2, 2019

Merge 24.6 and update version number to 24.6.0.
Thank you everyone to help me complete this PR.
I learned a lot from this experience.

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

Thanks a lot @WeiAnAn, I think this will make a lot of people in #6194 happy 😅

Sort test path alphabetically

```js
const Sequencer = require('@jest/test-sequencer').default;
Copy link
Member

Choose a reason for hiding this comment

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

This could do import Sequencer from '@jest/test-sequencer'; instead to avoid the .default part. Fine as-is though

@SimenB
Copy link
Member

SimenB commented Apr 2, 2019

Thanks @WeiAnAn!

@SimenB SimenB merged commit 2bc6280 into jestjs:master Apr 2, 2019
@@ -71,6 +71,7 @@ const defaultOptions: Config.DefaultOptions = {
testRegex: [],
testResultsProcessor: null,
testRunner: 'jasmine2',
testSequencer: '@jest/test-sequencer',
Copy link
Contributor

Choose a reason for hiding this comment

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

This value doesn't get fully resolved unless explicitly set, so it cannot work with PnP. It should be done like here: https://github.com/WeiAnAn/jest/blob/301a5cda50bf82617fe776938ee3fbd1c24b4a7c/packages/jest-config/src/normalize.ts#L477-L479

Copy link
Member

Choose a reason for hiding this comment

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

We resolve it in normalize already don't we?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm checking, but I think it only get resolved when the testSequence option is explicitly set - otherwise it uses the default value as-is.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants