Concurrent reporter + Run failed tests first #1480

Merged
merged 6 commits into from Sep 23, 2016

Conversation

Projects
None yet
7 participants
@aaronabramov
Member

aaronabramov commented Aug 23, 2016

react_reporter_demo

@ghost ghost added CLA Signed ✔️ labels Aug 23, 2016

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Aug 25, 2016

Current coverage is 87.44% (diff: 100%)

Merging #1480 into master will not change coverage

@@             master      #1480   diff @@
==========================================
  Files            38         38          
  Lines          1211       1211          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           1059       1059          
  Misses          152        152          
  Partials          0          0          

Powered by Codecov. Last update 6546bcb...309f50f

codecov-io commented Aug 25, 2016

Current coverage is 87.44% (diff: 100%)

Merging #1480 into master will not change coverage

@@             master      #1480   diff @@
==========================================
  Files            38         38          
  Lines          1211       1211          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           1059       1059          
  Misses          152        152          
  Partials          0          0          

Powered by Codecov. Last update 6546bcb...309f50f

@aaronabramov

This comment has been minimized.

Show comment
Hide comment
@aaronabramov

aaronabramov Aug 27, 2016

Member

question.
right now this reporter tries to print when it's run on travis, and the output looks like this
screen shot 2016-08-27 at 12 55 42 pm

is there a good way to detect if we run it inside a CI to avoid printing it?
process.stdin.isTTY does not really work.
should it be a module that can identify the most popular CI environment and return true/false?

Member

aaronabramov commented Aug 27, 2016

question.
right now this reporter tries to print when it's run on travis, and the output looks like this
screen shot 2016-08-27 at 12 55 42 pm

is there a good way to detect if we run it inside a CI to avoid printing it?
process.stdin.isTTY does not really work.
should it be a module that can identify the most popular CI environment and return true/false?

@maximderbin

This comment has been minimized.

Show comment
Hide comment
@maximderbin

maximderbin Aug 27, 2016

Contributor

@dmitriiabramov on circle ci they has ENV var ENV['CIRCLECI'] travis should probably have something the same

Contributor

maximderbin commented Aug 27, 2016

@dmitriiabramov on circle ci they has ENV var ENV['CIRCLECI'] travis should probably have something the same

@ghost ghost added the CLA Signed ✔️ label Aug 27, 2016

@aaronabramov

This comment has been minimized.

Show comment
Hide comment
@aaronabramov

aaronabramov Aug 28, 2016

Member

@maximderbin yeah... so i thought about making a package like jest-is-ci and having it return true if any of the env variables (like CIRLCECI, TRAVISCI) set. but i'm not sure if there's a better way to do it

Member

aaronabramov commented Aug 28, 2016

@maximderbin yeah... so i thought about making a package like jest-is-ci and having it return true if any of the env variables (like CIRLCECI, TRAVISCI) set. but i'm not sure if there's a better way to do it

@ghost ghost added the CLA Signed ✔️ label Aug 28, 2016

- for (let i = 0; i < string.length; i++) {
- process.stderr.write(string.charAt(i));
- }
+ process.stderr.write(string);

This comment has been minimized.

@alansouzati

alansouzati Aug 29, 2016

Contributor

nice! :)

@alansouzati

alansouzati Aug 29, 2016

Contributor

nice! :)

This comment has been minimized.

@cpojer

cpojer Sep 21, 2016

Contributor

@dmitriiabramov can you explain this? We had a huge issue in the past with previous node versions where on www the test run would just exit if there was a large amount of failures. You could potentially try this by failing 10 % of all assertions and running all tests on www. If the test run passes and prints big diffs, it is safe to remove it.

@cpojer

cpojer Sep 21, 2016

Contributor

@dmitriiabramov can you explain this? We had a huge issue in the past with previous node versions where on www the test run would just exit if there was a large amount of failures. You could potentially try this by failing 10 % of all assertions and running all tests on www. If the test run passes and prints big diffs, it is safe to remove it.

This comment has been minimized.

@aaronabramov

aaronabramov Sep 21, 2016

Member

i tested it on www, but not by failing that many tests.
i believe this line should have fixed it. (process.on('exit')).
we should definitely test it more thoroughly though.

@aaronabramov

aaronabramov Sep 21, 2016

Member

i tested it on www, but not by failing that many tests.
i believe this line should have fixed it. (process.on('exit')).
we should definitely test it more thoroughly though.

@probablyup

This comment has been minimized.

Show comment
Hide comment
@probablyup

probablyup Aug 30, 2016

Contributor

NODE_ENV="CI"?

Contributor

probablyup commented Aug 30, 2016

NODE_ENV="CI"?

@dirk

This comment has been minimized.

Show comment
Hide comment
@dirk

dirk Aug 31, 2016

@dmitriiabramov: Travis, Circle, and Buildkite all have CI=true in the environment. This is, I think, is the most reliable way to know when you're running on CI. 😉

dirk commented Aug 31, 2016

@dmitriiabramov: Travis, Circle, and Buildkite all have CI=true in the environment. This is, I think, is the most reliable way to know when you're running on CI. 😉

@ghost ghost added the CLA Signed ✔️ label Aug 31, 2016

@aaronabramov

This comment has been minimized.

Show comment
Hide comment
@aaronabramov

aaronabramov Sep 2, 2016

Member

@dirk seems like that's exactly what we need! thanks! :)

Member

aaronabramov commented Sep 2, 2016

@dirk seems like that's exactly what we need! thanks! :)

@ghost ghost added the CLA Signed ✔️ label Sep 2, 2016

@justingreenberg justingreenberg referenced this pull request in react-boilerplate/react-boilerplate Sep 10, 2016

Closed

RFC: Update tests to use Jest/Enzyme #970

@ghost ghost added the CLA Signed ✔️ label Sep 14, 2016

@ghost ghost added CLA Signed ✔️ labels Sep 14, 2016

@aaronabramov

This comment has been minimized.

Show comment
Hide comment
@aaronabramov

aaronabramov Sep 15, 2016

Member

screen shot 2016-09-15 at 2 29 17 pm

this also fixes the output in CI (except this 'running coverage' line, but that's not related to this change)

Member

aaronabramov commented Sep 15, 2016

screen shot 2016-09-15 at 2 29 17 pm

this also fixes the output in CI (except this 'running coverage' line, but that's not related to this change)

const getMaxWorkers = argv => {
if (argv.runInBand) {
return 1;
} else if (argv.maxWorkers) {
- return argv.maxWorkers;
+ return parseInt(argv.maxWorkers, 10);

This comment has been minimized.

@cpojer

cpojer Sep 21, 2016

Contributor

hahaha. How long did it take you to figure out that you need to do this so that throat won't explode? Me, in the past when I did experiments, I had to sleep over it.

@cpojer

cpojer Sep 21, 2016

Contributor

hahaha. How long did it take you to figure out that you need to do this so that throat won't explode? Me, in the past when I did experiments, I had to sleep over it.

This comment has been minimized.

@aaronabramov

aaronabramov Sep 21, 2016

Member

oh.. i actually thought it was an existing bug in jest :)

@aaronabramov

aaronabramov Sep 21, 2016

Member

oh.. i actually thought it was an existing bug in jest :)

This comment has been minimized.

@aaronabramov

aaronabramov Sep 21, 2016

Member

at least it didn't pass the flow types. i think it was number when specified in package.json and string if from CLI, or something like that

@aaronabramov

aaronabramov Sep 21, 2016

Member

at least it didn't pass the flow types. i think it was number when specified in package.json and string if from CLI, or something like that

@ghost ghost added CLA Signed ✔️ labels Sep 21, 2016

@cpojer cpojer changed the title from Concurrent reporter to Concurrent reporter + Run failed tests first Sep 23, 2016

@ghost ghost added CLA Signed ✔️ labels Sep 23, 2016

@ghost ghost added the CLA Signed ✔️ label Sep 23, 2016

@cpojer

This comment has been minimized.

Show comment
Hide comment
@cpojer

cpojer Sep 23, 2016

Contributor

Current Status:

jest-reporter

Contributor

cpojer commented Sep 23, 2016

Current Status:

jest-reporter

@ghost ghost added CLA Signed ✔️ labels Sep 23, 2016

@ghost ghost added the CLA Signed ✔️ label Sep 23, 2016

@cpojer cpojer merged commit 428565b into facebook:master Sep 23, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

mthmulders added a commit to mthmulders/jest that referenced this pull request Oct 10, 2016

Concurrent reporter + Run failed tests first (#1480)
* Concurrent reporter

* Cleanup and Fixes

* Add estimated runtime.

* Run failed tests first.

* Progress bar + printing cleanups.

* Polish printing.

@mpj mpj referenced this pull request in facebook/create-react-app Nov 29, 2016

Closed

Configure Jest to bail on first error #1108

@aaronabramov aaronabramov deleted the aaronabramov:concurrent-reporter branch Jun 8, 2017

tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017

Concurrent reporter + Run failed tests first (#1480)
* Concurrent reporter

* Cleanup and Fixes

* Add estimated runtime.

* Run failed tests first.

* Progress bar + printing cleanups.

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