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

Move test summary to after coverage report #4104 #4512

Merged

Conversation

hramezani
Copy link
Contributor

@hramezani hramezani commented Sep 19, 2017

Summary
Move test summary to after coverage report issue_4104

Test plan

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@SimenB
Copy link
Member

SimenB commented Oct 7, 2017

@hramezani This is failing lint (just yarn lint --fix and you should be good) and a single test on appveyor (which seems totally unreleated, maybe just rerun it? You can try rebasing on master and force pushing).

Can you add a test for this? Some sort of integration test with a snapshot of the output showing that it's placed correctly would be sweet 🙂

@hramezani hramezani force-pushed the summary_after_coverage_report_issue_4104 branch from f60798c to 74a3841 Compare October 7, 2017 16:30
@codecov-io
Copy link

Codecov Report

Merging #4512 into master will decrease coverage by <.01%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4512      +/-   ##
==========================================
- Coverage   56.18%   56.18%   -0.01%     
==========================================
  Files         194      194              
  Lines        6546     6548       +2     
  Branches        3        3              
==========================================
+ Hits         3678     3679       +1     
- Misses       2867     2868       +1     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-cli/src/test_scheduler.js 17.5% <75%> (+0.55%) ⬆️

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 7b0e108...74a3841. Read the comment docs.

@hramezani
Copy link
Contributor Author

@SimenB thanks,
I rebase my branch with master.
I want to add the test, can you give some example to me?

@SimenB
Copy link
Member

SimenB commented Oct 7, 2017

I'm somewhat confused that there are no broken tests from your change, to be honest...

Anyways, new tests should be added e.g. here. That test runs this "project" and makes assertions on the output. You should be able to assert that the coverage report is located at the correct place in the output.

@hramezani
Copy link
Contributor Author

@SimenB, I checked the tests in here.
the runJest return stdout, stderr, ... . there is no output from this function that returns all the output(summary and coverage) continuous.
for example in json reporter printing with --coverage test summary extract from strerr, and whet I get stdout from runJest, it contains summary report only.

How can I get all the output(summary and coverage) ?

@SimenB
Copy link
Member

SimenB commented Oct 8, 2017

Ah, that might explain it! Can you expand runJest to return it all?

@hramezani
Copy link
Contributor Author

@SimenB, I checked the runJest function. I think it is a wrapper and stdout and stderr are provided by jest.

@SimenB
Copy link
Member

SimenB commented Oct 13, 2017

That's correct. But stdout should include the coverage report

@hramezani
Copy link
Contributor Author

@SimenB , are you waiting for me to including coverage in stdout?

@SimenB
Copy link
Member

SimenB commented Oct 16, 2017

Either that or some other test showing that the change has the desired effect 🙂

@cpojer
Copy link
Member

cpojer commented Dec 15, 2017

@hramezani can you help us push this PR over the finish line? Thanks!

@hramezani
Copy link
Contributor Author

@cpojer, unfortunately, there is no way to test this feature, so I create a test project and install jest with this changes, then I run the tests with --coverage.

@cpojer
Copy link
Member

cpojer commented Dec 16, 2017

We have a folder called integration_tests which has multiple different Jest setups and then uses Jest to run those. We do a lot of tests like this, and yours can be done similarly. Mind checking out that folder and creating a test similar to it?

@hramezani
Copy link
Contributor Author

@cpojer , yes you right. @SimenB offers me as you in this comment to use integration_tests.
but there is a problem that I explained it in this comment

@SimenB
Copy link
Member

SimenB commented Dec 16, 2017

Is coverage output to stderr?

@cpojer cpojer merged commit e4d03fb into jestjs:master Feb 7, 2018
@cpojer
Copy link
Member

cpojer commented Feb 7, 2018

I'm gonna merge this just to get it out of the PR queue. The code changes look fine to me, and we have no existing tests that verify the order atm anyway.

@SimenB
Copy link
Member

SimenB commented Feb 7, 2018

@hramezani mind sending a PR for an update to the changelog? 🙂

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

5 participants