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

Conversation

Projects
None yet
7 participants
@BetterCallSky
Contributor

BetterCallSky 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

This comment has been minimized.

Show comment
Hide comment
@SimenB

SimenB Nov 14, 2017

Collaborator

This is failing CI

Collaborator

SimenB commented Nov 14, 2017

This is failing CI

@SimenB SimenB requested a review from rogeliog Nov 14, 2017

@SimenB

This comment has been minimized.

Show comment
Hide comment
@SimenB

SimenB Nov 14, 2017

Collaborator

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

Collaborator

SimenB commented Nov 14, 2017

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

@BetterCallSky

This comment has been minimized.

Show comment
Hide comment
@BetterCallSky

BetterCallSky Nov 14, 2017

Contributor

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.

Contributor

BetterCallSky 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

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io 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.

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

This comment has been minimized.

Show comment
Hide comment
@SimenB

SimenB Nov 23, 2017

Collaborator

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

Collaborator

SimenB commented Nov 23, 2017

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

Show outdated Hide outdated packages/jest-cli/src/__tests__/__snapshots__/watch.test.js.snap
@@ -60,5 +61,9 @@ export default (globalConfig: GlobalConfig, options: Options): GlobalConfig => {
newConfig.passWithNoTests = true;
}
if ('onlyFailures' in options) {
newConfig.onlyFailures = options.onlyFailures || false;

This comment has been minimized.

@rogeliog

rogeliog Nov 27, 2017

Collaborator

Do we need to cast onlyFailures to boolean?

@rogeliog

rogeliog Nov 27, 2017

Collaborator

Do we need to cast onlyFailures to boolean?

This comment has been minimized.

@BetterCallSky

BetterCallSky Dec 6, 2017

Contributor

I did that because of Flow warning.

@BetterCallSky

BetterCallSky Dec 6, 2017

Contributor

I did that because of Flow warning.

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

This comment has been minimized.

@rogeliog

rogeliog Nov 27, 2017

Collaborator

Should pressing C also clear this filter?

@rogeliog

rogeliog Nov 27, 2017

Collaborator

Should pressing C also clear this filter?

@@ -130,7 +130,14 @@ async function jasmine2(
},
});
if (globalConfig.testNamePattern) {
if (globalConfig.enabledTestsMap) {

This comment has been minimized.

@rogeliog

rogeliog Nov 27, 2017

Collaborator

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

@rogeliog

rogeliog Nov 27, 2017

Collaborator

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

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

This comment has been minimized.

@rogeliog

rogeliog Nov 27, 2017

Collaborator

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

@rogeliog

rogeliog Nov 27, 2017

Collaborator

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

@rogeliog

This comment has been minimized.

Show comment
Hide comment
@rogeliog

rogeliog Nov 27, 2017

Collaborator

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

Collaborator

rogeliog commented Nov 27, 2017

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

@SimenB

This comment has been minimized.

Show comment
Hide comment
@SimenB

SimenB Dec 6, 2017

Collaborator

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

Collaborator

SimenB commented Dec 6, 2017

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

@BetterCallSky

This comment has been minimized.

Show comment
Hide comment
@BetterCallSky

BetterCallSky Dec 6, 2017

Contributor

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

Contributor

BetterCallSky commented Dec 6, 2017

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

BetterCallSky added some commits Nov 13, 2017

add only-failures mode
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>
replace 'only-failures' message
Signed-off-by: Łukasz Sentkiewicz <sirmims@gmail.com>
@SimenB

SimenB approved these changes Dec 14, 2017

@cpojer cpojer merged commit 6f7c85a into facebook:master Dec 15, 2017

6 checks passed

ci/circleci: test-and-deploy-website Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
ci/circleci: test-node-6 Your tests passed on CircleCI!
Details
ci/circleci: test-node-8 Your tests passed on CircleCI!
Details
ci/circleci: test-node-9 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@cpojer

This comment has been minimized.

Show comment
Hide comment
@cpojer

cpojer Dec 15, 2017

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@apaatsio

apaatsio Jan 15, 2018

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.

apaatsio commented Jan 15, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment