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 only-failures mode #4886

Merged
merged 2 commits into from Dec 15, 2017
Merged

add only-failures mode #4886

merged 2 commits into from Dec 15, 2017

Conversation

lstkz
Copy link
Contributor

@lstkz lstkz commented Nov 13, 2017

Requested in #3634.
Solution:
When pressing f it re-runs only failed tests. Newly added tests are ignored.
See demo http://take.ms/g4Oyt

There are flow issues and I will fix it later. Let me know your feedback.

@SimenB
Copy link
Member

SimenB commented Nov 14, 2017

This is failing CI

@SimenB SimenB requested a review from rogeliog November 14, 2017 13:46
@SimenB
Copy link
Member

SimenB commented Nov 14, 2017

Can this be plugged in with the new approach introduced in #4841? /cc @wbinnssmith

@lstkz
Copy link
Contributor Author

lstkz commented Nov 14, 2017

In the current form, the plugin system doesn't allow it.
https://github.com/facebook/jest/pull/4886/files#diff-e4399d1682ddb42f8a43f1a917d42d87
This must be overridden in some way.

@codecov-io
Copy link

codecov-io commented Nov 14, 2017

Codecov Report

Merging #4886 into master will decrease coverage by 0.01%.
The diff coverage is 55.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4886      +/-   ##
==========================================
- Coverage   60.74%   60.72%   -0.02%     
==========================================
  Files         198      200       +2     
  Lines        6605     6638      +33     
  Branches        4        4              
==========================================
+ Hits         4012     4031      +19     
- Misses       2593     2607      +14
Impacted Files Coverage Δ
packages/jest-config/src/index.js 23.8% <ø> (ø) ⬆️
packages/jest-cli/src/constants.js 100% <ø> (ø) ⬆️
packages/jest-jasmine2/src/index.js 5.63% <0%> (-0.34%) ⬇️
packages/jest-cli/src/get_no_test_found_failed.js 0% <0%> (ø)
packages/jest-cli/src/run_jest.js 50.87% <33.33%> (-0.98%) ⬇️
packages/jest-cli/src/watch.js 74.82% <40%> (-1.3%) ⬇️
packages/jest-cli/src/get_no_test_found_message.js 60% <50%> (-6.67%) ⬇️
packages/jest-cli/src/lib/update_global_config.js 87.5% <50%> (-3.41%) ⬇️
packages/jest-cli/src/failed_tests_cache.js 87.5% <87.5%> (ø)
... and 1 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 39bfa34...ff08093. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Nov 23, 2017

@wbinnssmith Thoughts on the comment above about this feature not being possible to implement with the pluggable watch mode?

@@ -5,6 +5,7 @@ Array [
"
Watch Usage
› Press a to run all tests.
› Press f to enable \\"only-failures\\" mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we phrase it in a way that is more similar to "to run only failed tests" so that we don't expose the "only-failures" keyword?

@@ -60,5 +61,9 @@ export default (globalConfig: GlobalConfig, options: Options): GlobalConfig => {
newConfig.passWithNoTests = true;
}

if ('onlyFailures' in options) {
newConfig.onlyFailures = options.onlyFailures || false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to cast onlyFailures to boolean?

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 did that because of Flow warning.

@@ -224,6 +229,12 @@ export default function watch(
});
startRun(globalConfig);
break;
case KEYS.F:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should pressing C also clear this filter?

@@ -130,7 +130,14 @@ async function jasmine2(
},
});

if (globalConfig.testNamePattern) {
if (globalConfig.enabledTestsMap) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How would this work with jest-circus? cc: @aaronabramov @cpojer

return globalConfig;
}
// $FlowFixMe Object.assign
const newConfig: GlobalConfig = Object.assign({}, globalConfig);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be done through import updateGlobalConfig from './lib/update_global_config';

@rogeliog
Copy link
Contributor

It would be really nice if we make plugins robust enough to support this as an independent plugin 😄

@SimenB
Copy link
Member

SimenB commented Dec 6, 2017

@lsentkiewicz thoughts on the feedback? This also needs a rebase

@lstkz
Copy link
Contributor Author

lstkz commented Dec 6, 2017

@SimenB
Sorry for the delay, I will do it in 1-2 days.

Signed-off-by: Łukasz Sentkiewicz <sirmims@gmail.com>

lint fixes

Signed-off-by: Łukasz Sentkiewicz <sirmims@gmail.com>

fix tests

Signed-off-by: Łukasz Sentkiewicz <sirmims@gmail.com>

fix tests

Signed-off-by: Łukasz Sentkiewicz <sirmims@gmail.com>
Signed-off-by: Łukasz Sentkiewicz <sirmims@gmail.com>
@cpojer cpojer merged commit 6f7c85a into jestjs:master Dec 15, 2017
@cpojer
Copy link
Member

cpojer commented Dec 15, 2017

Let's do it for Jest 22. @lsentkiewicz thanks so much for sending this PR and thanks to all the reviewers for getting this into a good shape. I still have concerns that this will make people unaware of tests that start failing while in this mode that won't be captured by this, and that may be confusing for people. I'm fine with this because I think it still speeds up the process of fixing a bunch of tests even if you have to run all tests once at the end and fix up other stuff.

@apaatsio
Copy link

I still have concerns that this will make people unaware of tests that start failing while in this mode that won't be captured by this, and that may be confusing for people.

I share the same concern. I see how this can make fixing some bugs faster but I'm unsure if the benefit outweighs the risk of some failing tests going unnoticed.

This mode can also increase the user's cognitive load. Since it re-tests only the originally failing tests, the user has to remember which tests were failing to know what was actually tested.

After the tests pass it shows a text "No failed test found.". This can be misleading since there might actually be other failing tests. It would be more clear if after the tests have been fixed it would show the list of all the originally failing tests with a "PASS" label next to them.

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

None yet

7 participants