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

feat: add long test time threshold option #9366

Merged

Conversation

@Draco-Umbratilis
Copy link
Contributor

Draco-Umbratilis commented Jan 5, 2020

Summary

Adds slowTestThreshold option to configure what is considered to be a long running test. Defaults to 5 seconds since it was the original hardcoded value. Based on suggestions from #7220.

Fixes #7220

Test plan

I have added two new tests to get_result_header.test.js.

@codecov-io
Copy link

codecov-io commented Jan 5, 2020

Codecov Report

Merging #9366 into master will increase coverage by 0.02%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9366      +/-   ##
==========================================
+ Coverage   64.77%   64.79%   +0.02%     
==========================================
  Files         280      280              
  Lines       11987    11988       +1     
  Branches     2956     2955       -1     
==========================================
+ Hits         7764     7768       +4     
  Misses       3591     3591              
+ Partials      632      629       -3
Impacted Files Coverage Δ
packages/jest-config/src/Defaults.ts 100% <ø> (ø) ⬆️
packages/jest-config/src/normalize.ts 77.74% <ø> (ø) ⬆️
packages/jest-config/src/Descriptions.ts 100% <ø> (ø) ⬆️
packages/jest-config/src/index.ts 11.76% <ø> (ø) ⬆️
packages/jest-config/src/ValidConfig.ts 100% <ø> (ø) ⬆️
packages/jest-test-result/src/helpers.ts 0% <ø> (ø) ⬆️
packages/jest-runner/src/runTest.ts 2.17% <0%> (-0.03%) ⬇️
packages/jest-reporters/src/get_result_header.ts 66.66% <100%> (+19.04%) ⬆️

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 9419034...582775e. Read the comment docs.

@SimenB
Copy link
Collaborator

SimenB commented Jan 5, 2020

Thanks for the PR! Can we make this a reporter option instead?

@SimenB SimenB requested review from jeysal and thymikee Jan 5, 2020
Copy link
Collaborator

thymikee left a comment

Let's discuss it a bit more

@@ -46,7 +46,7 @@ export default (
: null;

const testDetail = [];
if (runTime !== null && runTime > 5) {
if (runTime !== null && runTime > globalConfig.longTestThreshold) {

This comment has been minimized.

@thymikee

thymikee Jan 7, 2020 Collaborator

As OP from the mentioned issue said, such config option makes sense for specific scenarios (like running e2e tests), or in Jest nomenclature "projects". Hence, putting it into globalConfig is not the best place.

I'd go with either putting this inside projectConfig or making it an option of a reporter as @SimenB suggested. I'm not 100% convinced where the responsibility lies, but for now I'd expect Jest to determine whether the test was slow and pass that information to the reporter to render, so I'm leaning towards projectConfig solution.

@Draco-Umbratilis Draco-Umbratilis force-pushed the Draco-Umbratilis:customizable-test-time-thresholds branch from d5aa73c to b0ba242 Jan 12, 2020
@Draco-Umbratilis
Copy link
Contributor Author

Draco-Umbratilis commented Jan 12, 2020

In hindsight globalConfig was probably the worst place to put it in. I have reworked the feature as @thymikee suggested, making the runner responsible for decision whether the test was slow and reporter just reacts to new slow flag in perfStats.

@joshuapinter
Copy link

joshuapinter commented Jul 28, 2020

Hey @Draco-Umbratilis, great job with this feature. Looks like there's some interest from @SimenB to get this merged in. Are you able to get this branch updated and pushed across the finish line, or do you want a hand with the last bit?

Lemme know as we're keen to get this merged in and used in our project, but I don't want to step on your toes here.

Thanks!

@Draco-Umbratilis
Copy link
Contributor Author

Draco-Umbratilis commented Jul 28, 2020

Hi @joshuapinter, I will try to update the branch later today. Lets see whether someone (@SimenB @thymikee ?) will have some more notes after that or not.

@joshuapinter
Copy link

joshuapinter commented Jul 28, 2020

👍 Beauty. Looks like you've done a great job with it.

@SimenB
SimenB approved these changes Jul 28, 2020
Copy link
Collaborator

SimenB left a comment

Forgot about this! 😅 I don't feel too strongly about making it a reporter option (mostly because it's sorta finicky as we have some magic around setting default reporters which might mess up here)

packages/jest-reporters/src/get_result_header.ts Outdated Show resolved Hide resolved
docs/Configuration.md Outdated Show resolved Hide resolved
@@ -259,7 +259,12 @@ async function runTestInternal(
result.numPendingTests +
result.numTodoTests;

result.perfStats = {end: Date.now(), start};
const end = Date.now();
result.perfStats = {

This comment has been minimized.

@SimenB

SimenB Jul 28, 2020 Collaborator

should we stick runtime or something on here as well so we don't have to do end-start in multiple places?

This comment has been minimized.

@SimenB

SimenB Jul 29, 2020 Collaborator

@Draco-Umbratilis just this comment and we should be good to go

This comment has been minimized.

@Draco-Umbratilis

Draco-Umbratilis Jul 29, 2020 Author Contributor

I found one occurrence in jest-test-sequencer and made the change also there. Reflected in tests and changelog, I hope it is ok.

This comment has been minimized.

@SimenB

SimenB Jul 29, 2020 Collaborator

Yeah, I like it!

Draco-Umbratilis and others added 4 commits Jan 12, 2020
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
@Draco-Umbratilis Draco-Umbratilis force-pushed the Draco-Umbratilis:customizable-test-time-thresholds branch from 99ad223 to e2a4d28 Jul 28, 2020
Draco-Umbratilis and others added 2 commits Jul 29, 2020
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
@SimenB
SimenB approved these changes Jul 29, 2020
Copy link
Collaborator

SimenB left a comment

Thanks!

@SimenB SimenB merged commit 93cde55 into facebook:master Jul 29, 2020
22 of 23 checks passed
22 of 23 checks passed
cleanup-runs
Details
Running TypeScript compiler & ESLint
Details
Node v10.x on ubuntu-latest
Details
Node v10.x on macOS-latest Node v10.x on macOS-latest
Details
Node v10.x on windows-latest
Details
Node v12.x on ubuntu-latest
Details
Node v12.x on macOS-latest
Details
Node v12.x on windows-latest
Details
Node v13.x on ubuntu-latest
Details
Node v13.x on macOS-latest
Details
Node v13.x on windows-latest
Details
Node v14.x on ubuntu-latest
Details
Node v14.x on macOS-latest
Details
Node v14.x on windows-latest
Details
Facebook CLA Check Contributor License Agreement is valid!
Details
ci/circleci: test-jest-circus Your tests passed on CircleCI!
Details
ci/circleci: test-node-10 Your tests passed on CircleCI!
Details
ci/circleci: test-node-12 Your tests passed on CircleCI!
Details
ci/circleci: test-node-13 Your tests passed on CircleCI!
Details
ci/circleci: test-node-14 Your tests passed on CircleCI!
Details
ci/circleci: test-or-deploy-website Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
facebook.jest #20200729.9 succeeded
Details
@joshuapinter
Copy link

joshuapinter commented Jul 30, 2020

Nice job @SimenB and @Draco-Umbratilis! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.