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

Initialize reporters early in the process #4480

Closed

Conversation

aaronabramov
Copy link
Contributor

@aaronabramov aaronabramov commented Sep 14, 2017

addresses #4471

  1. VerboseReporter is gone, and we're back to a single DefaultReporter that will print extra data in verbose mode. (i want to merge SummaryReproter back too at some point)
  2. We don't modify globalConfig anymore (globalConfig = Object.freeze(Object.assign({}, globalConfig, {verbose: true}));) i hope that was the last place.
  3. Reporting logic is gone from test_scheduler.js and now fully encapsulated in reporter_dispatcher.js
  4. All reporters are initialized right after we get globalConfig in jest cli

a few bad things:

  1. i broke the rerun feature in NotifyReporter. We have a sketchy implementation that passes an extra function to all reporters that reruns the build. This function has to be passed during runtime, which means it can't be a custom option for a single reporter.
  2. I have to pass verbose all over the place just because we don't know whether we run verbose until we get the number of test we're going to run. Which means we either mutate the immutable config (bad) or pass an extra option all over the place (still bad, but not as bad)

in the next PR i want to add a few extra reporter events. specifically configInitialized, contextsReady, and projectRunsComplete

@aaronabramov aaronabramov force-pushed the initialize_reporters_early branch 4 times, most recently from e226fb8 to 248da4f Compare September 14, 2017 17:24
@codecov-io
Copy link

Codecov Report

Merging #4480 into master will decrease coverage by 0.77%.
The diff coverage is 16.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4480      +/-   ##
==========================================
- Coverage   56.85%   56.08%   -0.78%     
==========================================
  Files         186      187       +1     
  Lines        6300     6330      +30     
  Branches        3        3              
==========================================
- Hits         3582     3550      -32     
- Misses       2717     2779      +62     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-runner/src/index.js 55% <ø> (ø) ⬆️
packages/jest-cli/src/reporter_dispatcher.js 0% <0%> (-25%) ⬇️
packages/jest-runner/src/test_worker.js 0% <0%> (ø) ⬆️
packages/jest-cli/src/run_jest.js 0% <0%> (ø) ⬆️
packages/jest-cli/src/test_scheduler.js 0% <0%> (-16.95%) ⬇️
packages/jest-runner/src/run_test.js 2.22% <0%> (ø) ⬆️
packages/jest-cli/src/reporters/notify_reporter.js 0% <0%> (-8.7%) ⬇️
...s/jest-cli/src/reporters/_group_tests_by_suites.js 100% <100%> (ø)
packages/jest-cli/src/should_use_verbose.js 100% <100%> (ø)
packages/jest-cli/src/watch.js 73.17% <50%> (-0.39%) ⬇️
... and 9 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 13e4ba2...b02429d. Read the comment docs.

@cpojer
Copy link
Member

cpojer commented Sep 15, 2017

Could you split this up into smaller commits and smaller PRs that make this easier to review?

  • PR for unrelated preparatory work
  • Merge Default and Verbose reporter

etc.?

@aaronabramov
Copy link
Contributor Author

will do!

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

4 participants