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

Env: Run tests using the same version used for development. #18703

Merged

Conversation

@epiqueras
Copy link
Contributor

epiqueras commented Nov 23, 2019

Description

@wordpress/env's wp-env start command takes a [ref] positional argument, which defaults to master, to specify a version of https://github.com/WordPress/WordPress to run. This was being respected for the development environment, but not for the tests environment, which resulted in E2E tests being ran on the default version shipped with the WordPress Docker container, i.e. not master with TwentyTwenty which our tests expect.

This PR addresses the issue by mounting an isolated copy of the checked out WordPress used for development, in the tests container, so that it can use the same version.

How has this been tested?

It was verified that after running wp-env start, both the development and tests containers are now running the same version of WordPress and all E2E tests pass as expected.

Types of Changes

Bug Fix: @wordpress/env now respects the specified WordPress version for both development and tests environments.

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .
@epiqueras epiqueras added this to the Future milestone Nov 23, 2019
@epiqueras epiqueras self-assigned this Nov 23, 2019
@epiqueras epiqueras requested review from noahtallen and WordPress/gutenberg-core Nov 23, 2019
@@ -33,6 +33,7 @@
},
"dependencies": {
"chalk": "^2.4.2",
"copy-dir": "^1.2.0",

This comment has been minimized.

Copy link
@aduth

aduth Nov 27, 2019

Member

We could potentially avoid pulling in a new dependency by reusing fs-extra (already a devDependencies in the root package.json), which is also promises-based (avoids need to promisify).

https://github.com/jprichardson/node-fs-extra/blob/HEAD/docs/copy.md

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 27, 2019

Author Contributor

I started that way, but opted to optimize for a leaner @wordpress/env. fs-extra is 10x bigger.

This comment has been minimized.

Copy link
@aduth

aduth Nov 27, 2019

Member

I started that way, but opted to optimize for a leaner @wordpress/env. fs-extra is 10x bigger.

That's fair. It's not as optimal for our project to have multiple dependencies capable of achieving the same goal, but good to be thinking about the consumer here. Might be a good reason to look to whether we should try to replace or remove our existing usage of fs-extra, which I know to be quite limited.

In fact, I think the only thing we really "need" it for is this directory removal:

gutenberg/bin/commander.js

Lines 246 to 247 in 598211e

await fs.remove( gitWorkingDirectoryPath );
await fs.remove( svnWorkingDirectoryPath );

Historically I might have used rimraf for this.

Apparently recursive is available as an experimental option now in the latest Node LTS, so we could consider that option as well: https://nodejs.org/dist/latest-v12.x/docs/api/fs.html#fs_fs_rmdir_path_options_callback

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 27, 2019

Author Contributor

Historically I might have used rimraf for this.

And we already have rimraf. I'll make a note to replace it.

Apparently recursive is available as an experimental option now in the latest Node LTS, so we could consider that option as well

When it becomes stable right?

This comment has been minimized.

Copy link
@aduth

aduth Nov 27, 2019

Member

Apparently recursive is available as an experimental option now in the latest Node LTS, so we could consider that option as well

When it becomes stable right?

Ehh, it doesn't bother me much. Well.. assuming that it's experimental in the sense of "The API signature may change in the future" and not "Your entire filesystem might be destroyed".

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 27, 2019

Author Contributor

Stability: 1 - Experimental. The feature is not subject to Semantic Versioning rules. Non-backward compatible changes or removal may occur in any future release. Use of the feature is not recommended in production environments.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 27, 2019

Author Contributor
* Promisified dependencies
*/
const copyDir = util.promisify( require( 'copy-dir' ) );
const wait = util.promisify( setTimeout );

This comment has been minimized.

Copy link
@aduth

aduth Nov 27, 2019

Member

TIL setTimeout[ require( 'util' ).promisify.custom ] is a thing.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 27, 2019

Author Contributor

Yeah, I think it used to be that you had to use setTimeout[ require( 'util' ).promisify.custom ] directly.

const commonVolumes = `
${ dependencyMappings }
- ${ cwd }${ cwdTestsPath }/e2e-tests/mu-plugins/:/var/www/html/wp-content/mu-plugins/
- ${ cwd }${ cwdTestsPath }/e2e-tests/plugins/:/var/www/html/wp-content/plugins/${ cwdName }-test-plugins/`;
const volumes = `
- ${ cwd }/../${ cwdName }-wordpress/:/var/www/html/${ commonVolumes }`;
const testsVolumes = `
- tests-wordpress:/var/www/html/${ commonVolumes }`;
- ${ cwd }/../${ cwdName }-tests-wordpress/:/var/www/html/${ commonVolumes }`;

This comment has been minimized.

Copy link
@aduth

aduth Nov 27, 2019

Member

I know it exists already for gutenberg-wordpress, but this directory creation one level up from the project root was very unexpected for me the first time I encountered it. Was there a reason to not include it as a directory within the project, like already exists with wordpress? For one thing, it makes future clean-up easier, since it's kept self-contained.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 27, 2019

Author Contributor

It would make the plugin volume recursive and sometimes Docker tripped up on that for some reason.

@@ -97,6 +103,31 @@ module.exports = {
// Some commit refs need to be set as detached.
await repo.setHeadDetached( ref );
}

// Duplicate repo for the tests container.
let stashed = true; // Stash to avoid copying config changes.

This comment has been minimized.

Copy link
@aduth

aduth Nov 27, 2019

Member

I wonder if, rather than this stash + filesystem copy, we could just create a clone from the repoPath.

In some brief testing in the Terminal, when I'm in gutenberg-wordpress, then run:

git clone . ../gutenberg-tests-wordpress/

... it creates a checkout of WordPress, at the same commit as in gutenberg-wordpress, excluding modifications in the working directory. Isn't this the same as what you're trying to achieve with the logic here?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 27, 2019

Author Contributor

I don't think it copies over things like a detached head ref from await repo.setHeadDetached( ref );.

@epiqueras epiqueras force-pushed the try/using-the-same-wp-version-for-dev-and-tests-envs branch from 411e3e8 to e8a2b5e Dec 6, 2019
@epiqueras epiqueras merged commit 325e4e6 into master Dec 6, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@epiqueras epiqueras deleted the try/using-the-same-wp-version-for-dev-and-tests-envs branch Dec 6, 2019
@youknowriad youknowriad modified the milestones: Future, Gutenberg 7.1 Dec 9, 2019
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.