-
Notifications
You must be signed in to change notification settings - Fork 798
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
Update jetpack extension tests to work with update to @wordpress dependencies #20326
Update jetpack extension tests to work with update to @wordpress dependencies #20326
Conversation
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. 🔴 Action required: Please add missing changelog entries for the following projects: Use the Jetpack CLI tool to generate changelog entries by running the following command: Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Backup plugin:
Jetpack plugin:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests pass for me.
I couldn't test the block changes to projects/plugins/jetpack/extensions/blocks/recurring-payments/controls.js
because I couldn't get this branch to build. (It kept complaining about global.window
being undefined in webpack in locale/index.js
So I tested the change onChange={ id => setMembershipAmount( id ) }
on the master branch for the Recurring payment plan controls.
It works as expected. I can add/change the payment plan for the selected payment block:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the several other failures that need fixing:
- Looks like the upgrade of
@wordpress/eslint-plugin
is making it complain about stuff in the e2e directory. linting: Do pnpm install in Jetpack E2E dir before eslint #20333 should fix that. pnpm build-search
is failing withI'm guessing that[3] [webpack-cli] Failed to load '/home/runner/work/jetpack/jetpack/projects/plugins/jetpack/tools/webpack.config.search.js' config [3] [webpack-cli] Error: Cannot find module '@wordpress/dependency-extraction-webpack-plugin/util'
@wordpress/dependency-extraction-webpack-plugin/util
was renamed or removed?pnpm build-production-client
is failing with[build-production-client] Message: [build-production-client] Module parse failed: Unexpected token (47:304)
pnpm build-extensions
is failing with several errors aboutexport 'isRTL' was not found in '@wordpress/i18n'
and one aboutTypeError: document.querySelectorAll is not a function
.
@@ -0,0 +1,5 @@ | |||
Significance: patch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You seem to have added this change entry for packages/backup rather than plugins/backup.
I think you should be able to just move this to projects/plugins/backup/changelog/update-internal-dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed - also removed some duplicate changelog entries - seems like the renovate bot added some of its own somehow
const input = within( borderPanel ).getAllByLabelText( 'Border radius' )[ 1 ]; | ||
const input = borderPanel.querySelector( 'input[type="number"]' ); | ||
input.focus(); | ||
fireEvent.change( input, { target: { value: '6' } } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's going on with the indentation here?
Also, if you don't mind teaching me a little, what's the difference between using userEvent.type()
and fireEvent.change()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's going on with the indentation here?
Some this file had a mix of spaces and tabs - not sure why linting didnt pick that up when first committed - should be fixed now, but adds a bit of noise to the PR.
what's the difference between using userEvent.type() and fireEvent.change()
userEvent is the recommended approach as it tries to do a better job of replicating the range of events that happen when a user interacts with an element. fireEvent is a bit lower level and fires only the event specified. In some situations userEvent doesn't seem to work, and gutenberg have changed something about that element that stopped userEvent working, wasn't able to work out what, and I checked gutenberg tests and the core team are using fireEvent to test this component so just copied their approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. (:
2a4a3d4
to
f3747db
Compare
This extension is used in place of all `@wordpress/i18n` imports throughout the js codebase. Relatively recently, the `i18n` package stated to export an `isRTL` function, and a lot of consumers use it. Since in Jetpack this package is replaced by this extension - that doesn't export `isRTL` - they were then breaking the build.
* Add JSDOM to mock document for emotion library * Exclude extensions/shared/i18n-to-php from its own webpack module remapping so some methods from real package can be imported and re-exported * Fix mapping to dependency-extraction-webpack-plugin util file
Sigh, hundreds of "Invalid hook call" errors. Looks like @wordpress/* now depend on react 17, so we have to update everything to that too. Working on it... |
Otherwise we get hundreds of "Invalid hook call" errors.
d1edca7
to
ce15092
Compare
Looks like the build is failing due to the check that the built JS passes ES5. It doesn't due to at least two issues:
We could just drop the ES5 check, or see what p9dueE-3fG-p2 comes up with. |
Thanks @anomiex for sorting out the remaining issues!
Is it an option to drop the ES5 check for this PR, and add a follow up issue for adding a bundle browser support CI check as a follow up issue? At a quick look around there are some options for checking a bundle for support at the basic es5 versus es6 level but not against a specific browser list, so I am guessing we would need to create something of our own for this, but there may be an existing solution that my searching didn't unearth. |
It looks like we could easily enough update it to an ES9 check for now. That's determined through trial and error: there's a spread operator in there somewhere that makes it fail in ES8. The es-check tool you linked has a feature request at yowainwright/es-check#93 requesting the ability to use browserslist to determine the version. As far as I can tell, the existing eslint invocation we have as a check does the same thing that es-check would do. All we'd need with either tool is a way to turn a browserslist into the appropriate ecmaVersion. I found eslint-plugin-compat that claims to check things versus the browser list, but it seems that it's about browser globals rather than ES language features. It depends on you passing the correct ecmaVersion to eslint. |
Babel does less transpilation now that @wordpress/browserslist-config dropped IE11. ES9 is the minimum version that passes now. Longer term we should replace that check with something better. See p9dueE-3fG-p2 for details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to go ahead and hit "approve" on this so we can merge it into the parent branch. Tests are passing and it seems at least a few people have poked at it in-browser to verify that things still work.
The failing "Changelogger use" check should pass when merged into the parent due to change entries added there.
Thanks for all the work you did to help make this happen! |
Changes proposed in this Pull Request:
Related to #19328
Does this pull request change what data or activity we track or use?
No
Testing instructions:
pnpm install
pnpm test-extensions
in theprojects/plugins/jetpack
dir and make sure all pass