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

Replace wp-scripts env usage with wp-env in CI. #20280

Merged
merged 8 commits into from
May 21, 2020
Merged

Replace wp-scripts env usage with wp-env in CI. #20280

merged 8 commits into from
May 21, 2020

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Feb 18, 2020

Description

Uses wp-env for running Travis e2e tests instead of wp-scripts env.

While the branch is named deprecate/env I haven't actually addressed any deprecations in this PR. I think that can be handled separately, I haven't renamed the branch as I think there's some useful history in the PR.

@gziolo
Copy link
Member

gziolo commented Feb 19, 2020

What about package.json file?

gutenberg/package.json

Lines 215 to 216 in 59b07cc

"test-unit-php": "wp-scripts env test-php",
"test-unit-php-multisite": "cross-env WP_MULTISITE=1 wp-scripts env test-php",

"env": "wp-scripts env",

@epiqueras - do you have some other open PRs that replace wp-scripts env with wp-env?

Edit: I see now #20090 from @noahtallen that should remove the need for running PHPUnit tests through wp-scripts.

@gziolo gziolo added the [Package] Env /packages/env label Feb 19, 2020
@epiqueras
Copy link
Contributor

@epiqueras - do you have some other open PRs that replace wp-scripts env with wp-env?

Mine are all stale but I am pretty sure @noahtallen plans on doing this after he lands #20090.

@github-actions
Copy link

github-actions bot commented Feb 20, 2020

Size Change: 0 B

Total Size: 1.12 MB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.39 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 6.93 kB 0 B
build/block-directory/style-rtl.css 790 B 0 B
build/block-directory/style.css 791 B 0 B
build/block-editor/index.js 105 kB 0 B
build/block-editor/style-rtl.css 10.8 kB 0 B
build/block-editor/style.css 10.8 kB 0 B
build/block-library/editor-rtl.css 7.17 kB 0 B
build/block-library/editor.css 7.17 kB 0 B
build/block-library/index.js 119 kB 0 B
build/block-library/style-rtl.css 7.48 kB 0 B
build/block-library/style.css 7.48 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 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 190 kB 0 B
build/components/style-rtl.css 17.1 kB 0 B
build/components/style.css 17.1 kB 0 B
build/compose/index.js 9.24 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.42 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.11 kB 0 B
build/edit-navigation/index.js 6.6 kB 0 B
build/edit-navigation/style-rtl.css 857 B 0 B
build/edit-navigation/style.css 856 B 0 B
build/edit-post/index.js 302 kB 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-site/index.js 12.8 kB 0 B
build/edit-site/style-rtl.css 5.22 kB 0 B
build/edit-site/style.css 5.22 kB 0 B
build/edit-widgets/index.js 7.73 kB 0 B
build/edit-widgets/style-rtl.css 4.59 kB 0 B
build/edit-widgets/style.css 4.59 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/index.js 44.6 kB 0 B
build/editor/style-rtl.css 5.06 kB 0 B
build/editor/style.css 5.06 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.64 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 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 711 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 226 B 0 B
build/list-reusable-blocks/style.css 226 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 616 B 0 B
build/nux/style.css 613 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.8 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.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@talldan
Copy link
Contributor Author

talldan commented Feb 20, 2020

@gziolo Still testing the Travis config at the moment.

Curiously there are two consistently failing tests indicating an issue with the media modal:

  ● adding inline tokens › should insert inline image

    TimeoutError: waiting for selector ".media-modal li[aria-label="213d0b3e-db83-4f23-b049-ed101be540ff"]" failed: timeout 30000ms exceeded

and

  ● Classic › should insert media, convert to blocks, and undo in one step

    TimeoutError: waiting for selector ".media-modal li[aria-label="5e354290-05fc-4699-8ee1-6fc80d864b34"]" failed: timeout 30000ms exceeded

Possibly an issue uploading files?

@gziolo
Copy link
Member

gziolo commented Feb 21, 2020

@talldan, is it using exactly the same version of WordPress to run e2e tests? Does it share the same configuration? I wouldn't be surprised if we discover some subtle differences.

@talldan
Copy link
Contributor Author

talldan commented Feb 24, 2020

I've rebased, there are more tests failing because SCRIPT_DEBUG seems to be true now in the test environment and several React errors that we know about are being thrown. Probably a good idea to fix those, so this might act as an impetus!

I won't be working for the rest of the week. If this seems like something someone would like to take further feel free to push some commits to this branch or use this as a basic for a separate PR.

@talldan
Copy link
Contributor Author

talldan commented Feb 24, 2020

Oh, the issue with uploads may be a permissions issue. The previous implementation had these lines that I abruptly removed:

# Create the upload directory with permissions that Travis can handle.
mkdir -p wordpress/src/wp-content/uploads
chmod 767 wordpress/src/wp-content/uploads

From what I understand, with wp-env, the WordPress codebase is copied into a folder like ~/.wp-env/dd609e38eea1d81e91e8be063e246d80, so it might be a challenge re-implementing this.

What do you think @noisysocks, does it sound like that might be the issue?

@noisysocks
Copy link
Member

I've rebased, there are more tests failing because SCRIPT_DEBUG seems to be true now in the test environment and several React errors that we know about are being thrown. Probably a good idea to fix those, so this might act as an impetus!

If you wanted to kick that can down the road you could set SCRIPT_DEBUG to false using something like echo '{ "config": { "SCRIPT_DEBUG": false } }' > wp-env.override.json.

What do you think @noisysocks, does it sound like that might be the issue?

Hmm, are you able to see the issue with uploading media when you run E2E tests using wp-env locally? It's hard to say what's going on exactly without more info. If possible, though, I'd prefer to fix the issue in wp-env instead of using a chmod in the Travis script.

wp-env start will create a wp-content/uploads directory when it runs docker-compose run cli wp core install. The cli container runs this as the user with UID 33 which is www-data in the wordpress container. So, in theory, the uploads directory should be owned by www-data meaning the server in the wordpress container can write into it.

If we aren't able to reproduce this locally, we could explore getting extra debug info using one or more of these methods:

  • Use wp-env start --debug, though I don't think it will show us anything useful in this case.
  • Set WP_DEBUG_LOG to true in .wp-env.override.json so that WordPress writes all runtime errors to wp-content/debug.log and then have Travis cat wp-content/debug.log as its last step.
  • Have Travis CI run ls -la wp-content so we can see what permissions uploads has.

@talldan
Copy link
Contributor Author

talldan commented Mar 4, 2020

Thanks for the info @noisysocks. e2e tests are now passing.

@gziolo
Copy link
Member

gziolo commented Mar 5, 2020

There is still something wrong with PHPUnit tests.

We are getting closer to sunset wp-scripts env 🎉

Great work you all 🥇

@talldan
Copy link
Contributor Author

talldan commented Mar 6, 2020

@gziolo One step closer! Not to see how we can progress #20090.

kienstra added a commit to getblocklab/block-lab that referenced this pull request May 3, 2020
@talldan
Copy link
Contributor Author

talldan commented May 20, 2020

I notice there's also #22303 now, which might supersede this.

I rebased this anyway, and E2E tests are still passing, though PHP unit tests are still failing, which I found surprising.

@noahtallen
Copy link
Member

Yeah that’s odd. PHP is already on wp-env and seems to be working on most other PRs!


// Ensure the tests uploads folder is writeable for travis,
// creating the folder if necessary.
const testsUploadsPath = path.join(
Copy link
Member

Choose a reason for hiding this comment

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

Should we chmod more broadly to allow other things to have write access (e.g. chmod all of wp-content)? See some discussion around #22454 (comment) in which a PHP test doesn't have access to what it wants

This may also remove the need for a different chmod I added in .travis.yml for the PHP test steps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems like a good call. I can put together a separate PR to cover it.

@noahtallen
Copy link
Member

Here's the phpunit error. It's actually something in wp-env:

> packages/env/bin/wp-env "start"

- 

✖ EACCES: permission denied, open '/home/travis/wp-env/ce047cf4da04af20491927334fb977c6/tests-WordPress/wp-config.php'

[Error: EACCES: permission denied, open '/home/travis/wp-env/ce047cf4da04af20491927334fb977c6/tests-WordPress/wp-config.php'] {

  errno: -13,

  code: 'EACCES',

  syscall: 'open',

  path: '/home/travis/wp-env/ce047cf4da04af20491927334fb977c6/tests-WordPress/wp-config.php'

}

This sounds awfully similar to this comment dealing with Ubuntu support: #20180 (comment). I think the workaround might just be to start the environment once (i.e. remove wp-env start from the phpunit test run in travis). But we still need to fix the underlying issue.

# Connect Gutenberg to WordPress.
npm run env connect
npm run env cli plugin activate gutenberg
echo '{ "config": { "SCRIPT_DEBUG": false, "WP_PHP_BINARY": "php" } }' > .wp-env.override.json
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 had to also add "WP_PHP_BINARY": "php" here, as the default config was being overwritten by the .wp-env.override file. Using the override file like this won't be very maintainable.

Perhaps looking into command line options for overriding individual settings could be an option. e.g.:

npm run wp-env start --config SCRIPT_DEBUG=false

Copy link
Member

Choose a reason for hiding this comment

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

we are also looking to add support for setting the config for different environments in .wp-env.json so that we can turn of SCRIPT_DEBUG etc for the tests environment without having to override anything.

@talldan talldan marked this pull request as ready for review May 21, 2020 04:09
@talldan
Copy link
Contributor Author

talldan commented May 21, 2020

This sounds awfully similar to this comment dealing with Ubuntu support: #20180 (comment). I think the workaround might just be to start the environment once (i.e. remove wp-env start from the phpunit test run in travis). But we still need to fix the underlying issue.

Thanks for the troubleshooting @noahtallen, removing the other calls to wp-env start solved things.

Tests are passing now 🎉

Copy link
Member

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

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

Since the tests are passing, I'm not sure if we need to do anything else for this PR specifically :)

It uncovered some things that would be good for wp-env in the future though:

  • fix linux environment issue
  • .wp-env.json should handle config for different environments
  • need better permissions in the wordpress install so that tests etc have write access to the filesystem

@talldan talldan merged commit 8ce6ad1 into master May 21, 2020
@talldan talldan deleted the deprecate/env branch May 21, 2020 04:26
@github-actions github-actions bot added this to the Gutenberg 8.2 milestone May 21, 2020
@talldan
Copy link
Contributor Author

talldan commented May 21, 2020

I've made a couple of issues:

#20180 already covers linux env issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Env /packages/env [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants