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

Concurrent reporter + Run failed tests first #1480

Merged
merged 6 commits into from
Sep 23, 2016

Conversation

aaronabramov
Copy link
Contributor

react_reporter_demo

@ghost ghost added the CLA Signed ✔️ label Aug 23, 2016
@ghost ghost added the CLA Signed ✔️ label Aug 24, 2016
@codecov-io
Copy link

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

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

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

@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);
Copy link
Contributor

Choose a reason for hiding this comment

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

nice! :)

Copy link
Member

Choose a reason for hiding this comment

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

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

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

@quantizor
Copy link
Contributor

NODE_ENV="CI"?

@dirk
Copy link

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

aaronabramov commented Sep 2, 2016

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

@aaronabramov
Copy link
Contributor Author

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);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 the CLA Signed ✔️ label Sep 21, 2016
@cpojer cpojer force-pushed the concurrent-reporter branch from 1d4fcad to 609da33 Compare September 23, 2016 02:10
@ghost ghost added the CLA Signed ✔️ label Sep 23, 2016
@cpojer cpojer changed the title Concurrent reporter Concurrent reporter + Run failed tests first Sep 23, 2016
@ghost ghost added the CLA Signed ✔️ label Sep 23, 2016
@cpojer cpojer force-pushed the concurrent-reporter branch from 082405a to 835e750 Compare September 23, 2016 06:17
@ghost ghost added the CLA Signed ✔️ label Sep 23, 2016
@cpojer cpojer force-pushed the concurrent-reporter branch from 864c3e9 to a811d5d Compare September 23, 2016 07:06
@ghost ghost added the CLA Signed ✔️ label Sep 23, 2016
@cpojer cpojer force-pushed the concurrent-reporter branch from a811d5d to 19c6a1c Compare September 23, 2016 07:53
@ghost ghost added the CLA Signed ✔️ label Sep 23, 2016
@cpojer cpojer force-pushed the concurrent-reporter branch from 19c6a1c to 6ccd878 Compare September 23, 2016 09:01
@cpojer
Copy link
Member

cpojer commented Sep 23, 2016

Current Status:

jest-reporter

@ghost ghost added the CLA Signed ✔️ label Sep 23, 2016
@cpojer cpojer force-pushed the concurrent-reporter branch from 6ccd878 to f42ac48 Compare September 23, 2016 09:12
@ghost ghost added the CLA Signed ✔️ label Sep 23, 2016
@cpojer cpojer force-pushed the concurrent-reporter branch from f42ac48 to fad0e0e Compare September 23, 2016 09:31
@cpojer cpojer force-pushed the concurrent-reporter branch from fad0e0e to 309f50f Compare September 23, 2016 09:34
@ghost ghost added the CLA Signed ✔️ label Sep 23, 2016
@cpojer cpojer merged commit 428565b into jestjs:master Sep 23, 2016
mthmulders pushed a commit to mthmulders/jest that referenced this pull request Oct 10, 2016
* Concurrent reporter

* Cleanup and Fixes

* Add estimated runtime.

* Run failed tests first.

* Progress bar + printing cleanups.

* Polish printing.
@aaronabramov aaronabramov deleted the concurrent-reporter branch June 8, 2017 18:01
tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017
* Concurrent reporter

* Cleanup and Fixes

* Add estimated runtime.

* Run failed tests first.

* Progress bar + printing cleanups.

* Polish printing.
@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 13, 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.

7 participants