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.

Sign up for GitHub

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

Jump to bottom

Testing: Add verification for Create Block to Continues Integration #23195

Merged
merged 7 commits into from
Jun 17, 2020
Merged

Testing: Add verification for Create Block to Continues Integration #23195

merged 7 commits into from
Jun 17, 2020

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Jun 16, 2020 •
edited
Loading

Description

We had recently two regressions for scaffolded JS and CSS code using ESNext template through npm init @wordpress/block:

At least one of them (JSDoc) was introduced by breaking changes introduced in @wordpress/scripts. The other could be just a bug introduced during refactoring. Regardless to that, it's been on my mind for some time and it was also discussed several times in the past in PRs and on WordPress Slack. It was also raised by @ocean90 earlier today while I already started working on this PR 😄 (#23188 (review)):

Should we add some tests to make sure that future changes to scripts/ESLint plugin can be detected earlier?

This PR adds GitHub action that should automate the process of verification changes that could impact the default block scaffolded with Create Block. It uses a script that validates whether npm init @wordpress/block works properly with the latest changes applied to the master branch. It purposefully avoids installing @wordpress/scripts package from npm when scaffolding a test block and uses the local package by executing everything from the root of the project.

To make it possible and also to give the same amount of control to user two new CLI options were introduced:

--no-wp-scripts and --wp-scripts to let users override the settings that template defines for supports for @wordpress/scripts package integration

How has this been tested?

The script that is used for automated testing can be also executed locally:

$ npm run test:create-block

New GitHub action that will run on every PR opened:

Screen Shot 2020-06-16 at 12 17 03

@gziolo gziolo added [Status] In Progress Tracking issues with work in progress [Tool] Create Block /packages/create-block [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Jun 16, 2020
@github-actions
Copy link

github-actions bot commented Jun 16, 2020 •
edited
Loading

Size Change: 0 B

Total Size: 1.12 MB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.27 kB 0 B
build/block-directory/style-rtl.css 955 B 0 B
build/block-directory/style.css 955 B 0 B
build/block-editor/index.js 106 kB 0 B
build/block-editor/style-rtl.css 10.7 kB 0 B
build/block-editor/style.css 10.7 kB 0 B
build/block-library/editor-rtl.css 7.85 kB 0 B
build/block-library/editor.css 7.86 kB 0 B
build/block-library/index.js 129 kB 0 B
build/block-library/style-rtl.css 8.02 kB 0 B
build/block-library/style.css 8.02 kB 0 B
build/block-library/theme-rtl.css 749 B 0 B
build/block-library/theme.css 751 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 195 kB 0 B
build/components/style-rtl.css 15.9 kB 0 B
build/components/style.css 15.9 kB 0 B
build/compose/index.js 9.6 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.45 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.17 kB 0 B
build/edit-navigation/index.js 8.26 kB 0 B
build/edit-navigation/style-rtl.css 1.04 kB 0 B
build/edit-navigation/style.css 1.04 kB 0 B
build/edit-post/index.js 303 kB 0 B
build/edit-post/style-rtl.css 5.6 kB 0 B
build/edit-post/style.css 5.6 kB 0 B
build/edit-site/index.js 16.6 kB 0 B
build/edit-site/style-rtl.css 3.13 kB 0 B
build/edit-site/style.css 3.13 kB 0 B
build/edit-widgets/index.js 9.33 kB 0 B
build/edit-widgets/style-rtl.css 2.54 kB 0 B
build/edit-widgets/style.css 2.54 kB 0 B
build/editor/editor-styles-rtl.css 486 B 0 B
build/editor/editor-styles.css 487 B 0 B
build/editor/index.js 44.8 kB 0 B
build/editor/style-rtl.css 3.82 kB 0 B
build/editor/style.css 3.82 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.72 kB 0 B
build/format-library/style-rtl.css 561 B 0 B
build/format-library/style.css 562 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.13 kB 0 B
build/list-reusable-blocks/style-rtl.css 537 B 0 B
build/list-reusable-blocks/style.css 537 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 681 B 0 B
build/nux/style.css 676 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@gziolo gziolo force-pushed the update/create-block-wp-scripts-control branch from 73b5393 to be58239 Compare June 16, 2020 09:16
@gziolo gziolo requested review from ocean90 and fabiankaegy June 16, 2020 09:18
@gziolo gziolo mentioned this pull request Jun 16, 2020
Merged
@gziolo gziolo changed the title Update/create block wp scripts control Testing: Add verification for Create Block to Continues Integration Jun 16, 2020
@gziolo gziolo self-assigned this Jun 16, 2020
@gziolo gziolo requested review from ajitbohra, nerrad and ntwb as code owners June 16, 2020 15:38
@fabiankaegy
Copy link
Member

@gziolo Whats the reasoning behind making the --scripts-enabled and --no-scripts-enabled two separate options? It feels more like a true|false setting to me since in theory with the approach we have now you could set both.

(love the setting though :) )

@gziolo
Copy link
Member Author

gziolo commented Jun 16, 2020

Whats the reasoning behind making the --scripts-enabled and --no-scripts-enabled two separate options? It feels more like a true|false setting to me since in theory with the approach we have now you could set both.

I wasn't fully happy about that as well, but apparently this is the recommended way of dealing with booleans in commander.js:
https://github.com/tj/commander.js/#other-option-types-negatable-boolean-and-flagvalue

You can specify a boolean option long name with a leading no- to set the option value to false when used. Defined alone this also makes the option true by default.

@gziolo gziolo force-pushed the update/create-block-wp-scripts-control branch from 62e9cd6 to 8c1c8b3 Compare June 16, 2020 15:45
@gziolo
Copy link
Member Author

gziolo commented Jun 16, 2020

I forced pushed to see if new GitHub action triggers and it looks like I didn't break anything :)

@gziolo gziolo removed the [Status] In Progress Tracking issues with work in progress label Jun 16, 2020
gziolo
gziolo commented Jun 16, 2020
View reviewed changes
.github/workflows/create-block.yml
on:
push:
paths:
- 'packages/**'
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm still unsure of what would be important here, maybe package*.json to cover the case when dependencies change. Although it would probably mean that one of the packages changed as well.

Copy link
Member

Choose a reason for hiding this comment

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

I initially thought about only adding @WordPress dependencies for create-block but it has none. 🙈

In this case probably all of

gutenberg/packages/scripts/package.json

Lines 36 to 42 in cb10e2d

"@wordpress/babel-preset-default": "file:../babel-preset-default",
"@wordpress/dependency-extraction-webpack-plugin": "file:../dependency-extraction-webpack-plugin",
"@wordpress/eslint-plugin": "file:../eslint-plugin",
"@wordpress/jest-preset-default": "file:../jest-preset-default",
"@wordpress/npm-package-json-lint-config": "file:../npm-package-json-lint-config",
"@wordpress/postcss-plugins-preset": "file:../postcss-plugins-preset",
"@wordpress/prettier-config": "file:../prettier-config",
, packages/scripts/package.json and packages/create-block?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is also @wordpress/browserlist-config or @wordpress/element that is used by Babel. I said, it might be tricky to identify all dependencies used :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Once we add new configs, like @wordpress/styleling-config as proposed by @ntwb in #22777, we would have to update this list. I would personally prefer to extend the list of excluded files rather than allow list.

Copy link
Member

Choose a reason for hiding this comment

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

Let's use the current list and see how it goes.

ocean90
ocean90 reviewed Jun 16, 2020
View reviewed changes
bin/test-create-block.sh Outdated Show resolved Hide resolved
@gziolo gziolo force-pushed the update/create-block-wp-scripts-control branch from 6a16eea to 2dcfc7c Compare June 17, 2020 05:42
@gziolo
Copy link
Member Author

gziolo commented Jun 17, 2020 •
edited
Loading

Travis failures for e2e tests are unrelated to changes in this PR. I'll merge it regardless Travis complaints.

@gziolo gziolo merged commit 81819c2 into master Jun 17, 2020
@gziolo gziolo deleted the update/create-block-wp-scripts-control branch June 17, 2020 12:00
@github-actions github-actions bot added this to the Gutenberg 8.4 milestone Jun 17, 2020
This was referenced Jun 24, 2020
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ocean90 ocean90 ocean90 left review comments

@karmatosed karmatosed Awaiting requested review from karmatosed

@mapk mapk Awaiting requested review from mapk

@youknowriad youknowriad Awaiting requested review from youknowriad

@fabiankaegy fabiankaegy Awaiting requested review from fabiankaegy

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@nerrad nerrad Awaiting requested review from nerrad

@ntwb ntwb Awaiting requested review from ntwb

Assignees

@gziolo gziolo

Labels
[Tool] Create Block /packages/create-block [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Milestone
Gutenberg 8.4
Development

Successfully merging this pull request may close these issues.
3 participants
@gziolo @fabiankaegy @ocean90