Skip to content

Conversation

willdurand
Copy link
Member

@willdurand willdurand commented Aug 7, 2018

Fix mozilla/addons#10682
⚠️ depends on #5871


That works on my machine™️ Maybe we should test this reporter, but I don't really know how. I guess we'll notice when things will break :D

@willdurand willdurand force-pushed the test-reporter branch 2 times, most recently from 33bf069 to 1bb0447 Compare August 7, 2018 22:00
@codecov-io
Copy link

codecov-io commented Aug 7, 2018

Codecov Report

Merging #5872 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    mozilla/addons-frontend#5872      +/-   ##
==========================================
+ Coverage   97.72%   97.72%   +<.01%     
==========================================
  Files         228      228              
  Lines        5763     5764       +1     
  Branches     1105     1105              
==========================================
+ Hits         5632     5633       +1     
  Misses        116      116              
  Partials       15       15
Impacted Files Coverage Δ
src/amo/sagas/collections.js 100% <0%> (ø) ⬆️

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 7e0c461...ea33d5e. Read the comment docs.

@willdurand willdurand requested a review from kumar303 August 7, 2018 22:09
@willdurand
Copy link
Member Author

rebased.

Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

I say this is ready to ship once it has a name.

This is pretty good! Some notes:

  • I wanted it to only show the output for a failing test but it shows the output for the whole suite. This is better than before but less than ideal.
  • When I run a single test (in watch mode I type t and then filter by test spec), it doesn't show any output at all. This is what was mentioned in jestjs/jest#6441 . I chased this down a bit until I finally realized that without your custom reporter, it still doesn't even show output. So, yeah, it's a bummer but this means at least your reporter isn't making things worse.
  • The buffering code in Jest is really complicated so I don't know if we'll ever get grouped output as requested in jestjs/jest#2080 .

jest.config.js Outdated
// Replaces the following formats with an empty module.
'^.+\\.(scss|css|svg|woff|woff2|mp4|webm)$': '<rootDir>/tests/emptyModule',
},
reporters: ['<rootDir>/tests/fingers-crossed-reporter.js'],
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's ready for a proper name! I'd suggest putting it in a directory called tests/jest-reporters/

Copy link
Member Author

@willdurand willdurand Aug 8, 2018

Choose a reason for hiding this comment

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

Isn't it already a proper name? We hope we won't need to output console logs, that's the idea behind this name :) It is borrowed from https://github.com/Seldaek/monolog/blob/master/src/Monolog/Handler/FingersCrossedHandler.php#L19

@willdurand
Copy link
Member Author

willdurand commented Aug 8, 2018

I wanted it to only show the output for a failing test but it shows the output for the whole suite. This is better than before but less than ideal.

Yep I saw that too, but it is better than absolutely all logs. We don't have too many test cases per test suite. It is a baby step toward a better developer experience.

Copy link
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

:shipit:

I thought the name was just a placeholder while you got it working but I get it now. The comment above the reporter explains what it does so 🤷‍♀️

@willdurand willdurand merged commit 6bbf218 into master Aug 8, 2018
@willdurand willdurand deleted the test-reporter branch August 8, 2018 16:50
@Scimonster
Copy link

Maybe consider spinning this off into its own module so other people can easily take advantage of it?

@willdurand
Copy link
Member Author

Maybe consider spinning this off into its own module so other people can easily take advantage of it?

Not sure, it works well for us in this project but I am not sure it is generic enough to be reused elsewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jest test output is too big because of console logging
4 participants