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 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
Member

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 January 5, 2020 20:04
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.

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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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

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

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

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.

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 = {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I like it!

@Draco-Umbratilis Draco-Umbratilis force-pushed the customizable-test-time-thresholds branch from 99ad223 to e2a4d28 Compare July 28, 2020 18:59
Draco-Umbratilis and others added 2 commits July 29, 2020 21:02
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
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!

@joshuapinter
Copy link

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

@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.

Custom threshold for test time warning/errors
6 participants