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

Test: Collapse passed tests in Travis. #16755

Merged
merged 3 commits into from Dec 3, 2019

Conversation

@epiqueras
Copy link
Contributor

epiqueras commented Jul 25, 2019

Closes #16744

Description

This PR replaces the default jest reporter with a subclass that folds passed tests when running in Travis CI.

How has this been tested?

Tests were run locally and the default reporter was used. Now we need to verify the new reporter works as expected in Travis.

Types of Changes

Build Tooling: Collapse passed tests in Travis.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Jul 25, 2019

Screen Shot 2019-07-25 at 1 48 17 PM

It works! 🎉

@epiqueras epiqueras force-pushed the try/collapsing-passed-tests-in-travis branch 3 times, most recently from 1191e78 to 7aaf678 Jul 25, 2019
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Jul 25, 2019

Screen Shot 2019-07-25 at 3 08 11 PM

Now the fold has an actual description/header 🚀

Copy link
Contributor

nerrad left a comment

Nice! The only question I have with this is whether it's possible to (or should) be included in the @wordpress/jest-preset-defaults package instead? That way the alternative reporter is available in a wider scope?

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Jul 26, 2019

Nice! The only question I have with this is whether it's possible to (or should) be included in the @wordpress/jest-preset-defaults package instead? That way the alternative reporter is available in a wider scope?

I added it to all the test suites in this repo. I don't know if we want it to be part of the package that other repos might use or if it's something that should be specific to this one. Personally, I do think that having it in the package is a good idea.

We can also always easily move it later.

cc @youknowriad

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jul 26, 2019

Having it in the preset works for me 👍

@epiqueras epiqueras force-pushed the try/collapsing-passed-tests-in-travis branch from 7aaf678 to be26416 Jul 26, 2019
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Jul 26, 2019

Done 🎉

@@ -23,5 +23,6 @@
"transform": {
"^.+\\.[jt]sx?$": "<rootDir>/node_modules/babel-jest"
},
"verbose": true
"verbose": true,
"reporters": [ "../../../@wordpress/jest-preset-default/scripts/travis-fold-passes-reporter.js" ]

This comment has been minimized.

Copy link
@nerrad

nerrad Jul 26, 2019

Contributor

hmm... is this path right?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jul 26, 2019

Author Contributor

Yes, <rootDir> does not work there in .json files for some reason.

This comment has been minimized.

Copy link
@gziolo

gziolo Jul 26, 2019

Member

Did you test it outside of Gutenberg repository? In other places we use <rootDir>, e.g.:
<rootDir>/node_modules/@wordpress/jest-preset-default/scripts/setup-globals.js

As far as I remember this is the only way to make it work for Jest presets. At least this was the case in the past.

This comment has been minimized.

Copy link
@nerrad

nerrad Jul 26, 2019

Contributor

huh, I would have thought the path would be relative to the jest-preset.json file. I'm just wondering how well/if this will work things consuming this package (other than gutenberg). So with this being a relative path, would ./scripts/travis-fold-passes-reporter.js not work?

This comment has been minimized.

This comment has been minimized.

Copy link
@gziolo

gziolo Jul 26, 2019

Member

It looks like they migrated to jest-preset.js in this PR: thymikee/jest-preset-angular#204. I assume Jest improved the handling for presets.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jul 26, 2019

Author Contributor

Still doesn't replace <rootDir>.

This comment has been minimized.

Copy link
@aduth

aduth Nov 22, 2019

Member

@gziolo might be suggesting to resolve it using something like require.resolve in JavaScript?

require.resolve( '@wordpress/jest-preset-default/scripts/travis-fold-passes-reporter' )

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 23, 2019

Author Contributor

We should use <rootDir>/node_modules/@wordpress/jest-preset-default/scripts/... like with the other paths in this file, but first we need to update the package so that the new reporter file is present in the directory in node_modules.

This comment has been minimized.

Copy link
@gziolo

gziolo Nov 24, 2019

Member

I don't remember :) The way it's implemented at the moment, it won't work when installed from npm for all projects using wp-scripts test-unit-js.

@epiqueras epiqueras requested a review from aduth Nov 20, 2019
Copy link
Member

aduth left a comment

In this branch, when I run tests locally, I see the "Pass" label, but no details of the specific tests.

This is without any TRAVIS environment variable.

 npm run test-unit packages/blocks/src/api/test/validation.js

> gutenberg@6.9.0 test-unit /Users/andrew/Documents/Code/gutenberg
> wp-scripts test-unit-js --config test/unit/jest.config.js "packages/blocks/src/api/test/validation.js"

 PASS  packages/blocks/src/api/test/validation.js
@@ -23,5 +23,6 @@
"transform": {
"^.+\\.[jt]sx?$": "<rootDir>/node_modules/babel-jest"
},
"verbose": true
"verbose": true,
"reporters": [ "../../../@wordpress/jest-preset-default/scripts/travis-fold-passes-reporter.js" ]

This comment has been minimized.

Copy link
@aduth

aduth Nov 22, 2019

Member

@gziolo might be suggesting to resolve it using something like require.resolve in JavaScript?

require.resolve( '@wordpress/jest-preset-default/scripts/travis-fold-passes-reporter' )
@epiqueras epiqueras force-pushed the try/collapsing-passed-tests-in-travis branch from be26416 to 8d8024c Nov 23, 2019
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Nov 23, 2019

In this branch, when I run tests locally, I see the "Pass" label, but no details of the specific tests.

Good catch. We use verbose: true so I should have extended and exported the VerboseReporter instead of the DefaultReporter, try it now.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Nov 24, 2019

Can this be simplified to conditionally override the reported in this config file:
https://github.com/WordPress/gutenberg/blob/master/test/unit/jest.config.js?

This way this change will be isolated to Gutenberg. This will prevent enforcing the verbose reporter when running locally, make the issue with providing the path to the custom reporter no longer present.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Nov 24, 2019

This will prevent enforcing the verbose reporter when running locally

We already do this with verbose: true.

make the issue with providing the path to the custom reporter no longer valid

This is only temporary, until the file is in npm/node_modules.

@@ -27,5 +27,6 @@
"transform": {
"^.+\\.[jt]sx?$": "<rootDir>/node_modules/babel-jest"
},
"verbose": true
"verbose": true,

This comment has been minimized.

Copy link
@aduth

aduth Dec 2, 2019

Member

With the latest changes, I wonder if this line is even necessary. And then, it gets me considering, if someone was to use this preset as a base, and then override "verbose": false, would it actually disable the VerboseRunner from being used?

It seems like something where the additional reporter shouldn't be making any assumptions outside its own custom behavior, which we could do if we included "default" in the reporters array:

https://jestjs.io/docs/en/configuration#reporters-arraymodulename--modulename-options

I suppose the problem then is: What would the export of travis-folder-passes.reporter.js be in the else condition of 'TRAVIS' in process.env ?

I could see a few options:

  • Move the condition into each of the onTestResult and onRunComplete to treat them as a noop.
  • Maybe it's enough to revert back to extending DefaultReporter if we include 'default' in the reporters configuration?
  • Or, we could convert this file to a JavaScript file, and conditionally include the custom reporter path based on the process.env condition.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Dec 2, 2019

Author Contributor

Move the condition into each of the onTestResult and onRunComplete to treat them as a noop.

We still need to extend either the verbose or default reporter to get some output without running 2 reporters in Travis.

Maybe it's enough to revert back to extending DefaultReporter if we include 'default' in the reporters configuration?

Likewise, that would run the 2 reporters.

Or, we could convert this file to a JavaScript file, and conditionally include the custom reporter path based on the process.env condition.

That would work.

A potentially easier solution that respects the actual configuration would be to read the current Jest config, which reporters are constructed with:

https://github.com/facebook/jest/blob/ad5377333daf6716af3465bba39f86b7db485e2b/packages/jest-reporters/src/verbose_reporter.ts#L25

This comment has been minimized.

Copy link
@epiqueras

epiqueras Dec 2, 2019

Author Contributor

Seems to work: f7c72e7.

This comment has been minimized.

Copy link
@aduth

aduth Dec 3, 2019

Member

I think it can work. It still doesn't quite sit well with me, since it makes assumptions about what the default behavior would be from our custom code, rather than just letting Jest deal with the defaults. Converting jest-preset.json to a JavaScript file and manipulating the reporters array feels like a more durable long-term solution. I'm fine to explore that separately.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Dec 3, 2019

Author Contributor

Agreed.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Dec 3, 2019

Author Contributor
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Dec 3, 2019

In the failing end-to-end test (which itself doesn't appear to be related), I see that the passes aren't collapsed. Do we use a separate configuration for this?

See: https://travis-ci.com/WordPress/gutenberg/jobs/262393864

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 3, 2019

In the failing end-to-end test (which itself doesn't appear to be related), I see that the passes aren't collapsed. Do we use a separate configuration for this?

It looks like E2E tests are using the jest-puppeteer preset instead of our @wordpress/jest-preset-default.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 3, 2019

Can we update that in a follow-up PR?

@aduth
aduth approved these changes Dec 3, 2019
Copy link
Member

aduth left a comment

Sure

@epiqueras epiqueras merged commit ed8b4a5 into master Dec 3, 2019
1 of 2 checks passed
1 of 2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Failed
Details
@epiqueras epiqueras deleted the try/collapsing-passed-tests-in-travis branch Dec 3, 2019
@youknowriad youknowriad modified the milestones: Future, Gutenberg 7.1 Dec 9, 2019
scruffian added a commit to scruffian/gutenberg that referenced this pull request Dec 10, 2019
* Test: Collapse passed tests in Travis.

* Jest Preset Default: Use verbose reporter and simplify code.

* Jest Preset Default: Respect actual verbose setting.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.