Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Improve Jest error detection and reporter output (#1615) #1948

Merged
merged 2 commits into from
Nov 1, 2019

Conversation

zetlen
Copy link
Contributor

@zetlen zetlen commented Oct 30, 2019

Description

Improve Jest exception handling and clean up Jest reporter output.

  • Remove fileMock from venia-concept, since it's used in venia-ui now.
  • Add env setup file which suppresses console output from application code, but allows console output from tests themselves for debugging purposes (don't leave console output in your tests please)
  • Add custom environments for both jsdom and nodejs tests which capture unhandled promise rejections and fail tests if they exist
  • Correct the tests that were now failing:
    • packages/pwa-buildpack/lib/Utilities/__tests__/graphQL.spec.js was testing promise rejections with statements like expect(promise).rejects. In Jest you need to add another assertion after .rejects.
    • packages/venia-concept/src/__tests__/index.spec.js was trying to re-require the module in a way that caused unhandled rejections. Refactored the implementation for testability and simplified the test.
    • packages/venia-ui/lib/components/ProductFullDetail/__tests__/productFullDetail.spec.js was mocking a React component in a way that made it render undefined, which was causing a problem in react-test-renderer.

Notes:

  • Unhandled promise rejections should be test failures. This solution isn't perfect, but we're trying to get there.
  • If you want console output, you can either put console output in the test itself, or set the environment variables NODE_DEBUG to DEBUG to a non-empty value.
  • Please don't leave console output in your test files. Console output in your app files themselves will be suppressed, so you don't have to worry about that.

Related Issue

Closes #1615.

Acceptance

Verification Stakeholders

All core devs and QA:

Verification Steps

  1. Check out the branch and run a clean install.
  2. Run yarn test.
  3. Observe clean output without messy console logging.
  4. Run NODE_DEBUG=1 yarn test.
  5. Observe all the console output like before.
  6. Edit a test and break it: remove an await from an asynchronous test to cause an unhandled rejection.
  7. Run yarn run test:dev.
  8. Observe watch mode showing broken test.
  9. Correct the test and save the file.
  10. Observe watch mode recovering.

Optional:

  1. Add a console.log statement to a test file, helper file, or mock file.
  2. Run yarn test.
  3. Observe that the console message appears in reporter output even though the others are suppressed.

Checklist

  • I have updated the documentation accordingly, if necessary.
  • I have added tests to cover my changes, if necessary.

@zetlen zetlen added the test This pertains to testing (unit/functional/etc). label Oct 30, 2019
@zetlen zetlen changed the title Zetlen/jest improve everything Improve Jest error detection and reporter output (#1615) Oct 30, 2019
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Oct 30, 2019

Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).

Generated by 🚫 dangerJS against 028cb5e

@zetlen zetlen added the version: Patch This changeset includes backwards compatible bug fixes. label Oct 30, 2019
);
}
// This is stored as a global for testing purposes.
window.__registerServiceWorker = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this PR changes things, that have been moved to different files as part of #1905

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a fan of custom globals though __ prefix is OK I guess.

Is there not another way around this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's ugly, but it's incredibly common to have a custom global in an app like this one that takes over the whole document. And it does have the advantage that something else can remotely unsubscribe the handler, because there's a real reference to the handler: window.removeEventListener('load', window.__registerServiceWorker) is something that a future Webpack hot module reloading strategy could use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@revanth0212 Once one of these is merged, we'll rebase the other one!

Copy link
Contributor

@sirugh sirugh left a comment

Choose a reason for hiding this comment

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

I have issue with registering a global, especially for tests.
I also want to see some docs around the (necessary?) unhandled rejection handler.

Anyways, verification steps work. I think this is definitely better and cleaner. Woo!

@@ -0,0 +1,58 @@
/**
* Spanish console magic. Quiets the console during test reporting, except
* for console methods called _within tests themselves._
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ugly crying. This is great!

Great documentation :)

const tick = () => new Promise(setImmediate);

module.exports = Base =>
class RejectionHandlingEnv extends Base {
Copy link
Contributor

Choose a reason for hiding this comment

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

This class feels unnecessary -- why not just fix the tests in the first place?

And if we want to keep this I think we should add some docs around this explaining its purpose.

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 can add comments, but I do want to keep it.

The reason I added this is that the tests weren't failing, so they just flew past in the console logging and the test command exited clean. If you're in a hurry or you're unfamiliar with our test command output, you won't know there are tests to fix!

This doesn't allow tests to have unhandled promise rejections in them; instead, it actually forbids them by forcing a test containing them to fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you so much for the explanation and docs!

test: cleanup and improve jest output
@zetlen zetlen force-pushed the zetlen/jest-improve-everything branch from 61710f0 to d5d6560 Compare October 31, 2019 20:05
@dpatil-magento dpatil-magento merged commit 6827951 into develop Nov 1, 2019
@dpatil-magento dpatil-magento deleted the zetlen/jest-improve-everything branch November 1, 2019 18:44
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
pkg:pwa-buildpack pkg:venia-concept pkg:venia-ui test This pertains to testing (unit/functional/etc). version: Patch This changeset includes backwards compatible bug fixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cleanup Test Output
5 participants