Testing: Disable post locking in E2E tests

[aduth]

This pull request seeks to improve E2E test stability when running tests locally. I had experienced that when running the tests multiple times, I often witnessed that the trashExistingPosts function would silently fail when attempting to mass-trash posts if there was a post lock remaining from the previous test run. By default, post locks last 150 seconds and prevent posts from being removed via bulk trash.

Implementation notes:

Two notes:

• I had seen some feedback before about not using mu-plugins for E2E test plugins, specifically around the implementation of disable-animations.php (Add a plugin to disable the CSS animation for more stable e2e tests #13769 (comment)). It's not very clear to me why these plugins would be loaded into a non-E2E context? cc @youknowriad @gziolo
• Between this and Testing: Trash existing posts as admin user #14244, using bulk edit to trash posts seems fairly unreliable due to silent failures. I wonder if there might be a better way to reset the site, and in a way which removes redundancy with bin/reset-e2e-tests.sh doing the same.

Testing instructions:

Verify that E2E tests pass:

npm run test-e2e

[jorgefilipecosta]

Hi @aduth,
Would it make sense to have an end to end test for the post locking itself? If yes this plugin may make that harder.
If disabling post locking is only important during the bulk trash, would it be possible that this plugin is a test plugin that we enable before the bulk trash and disable after bulk trash is complete?

[youknowriad]

Would it make sense to have an end to end test for the post locking itself?

I tried but it's very hard to achieve. you need to switch browser contexts to do so, I did it by adding a page argument to all of our utilities but in the end the test was still unstable and I couldn't figure out why, so I abandoned. It doesn't mean we can't try again.

To fix the issue this mu plugin might introduce for this test, we could make it a regular plugin and enable it at the beginning of the test (like the no-animations one) . That way, a post locking test could disable the plugin during the test.

[gziolo]

• I had seen some feedback before about not using mu-plugins for E2E test plugins, specifically around the implementation of disable-animations.php (#13769 (comment)). It's not very clear to me why these plugins would be loaded into a non-E2E context?

As noted in the referenced comment. At the moment mu-plugins folder is scanned to load plugins not only for e2e tests but also for regular WordPress instance some people (including me) us for development:
https://github.com/WordPress/gutenberg/blob/master/docker-compose.yml#L16

If we want to continue using mu-plugins to disable certain features of WordPress we should only load them for e2e tests setup.

[aduth]

At the moment mu-plugins folder is scanned to load plugins not only for e2e tests but also for regular WordPress instance some people (including me) us for development: [...] If we want to continue using mu-plugins to disable certain features of WordPress we should only load them for e2e tests setup.

But, is there a reason it's done this way? Those plugins, evidenced by the path, should apply only in the E2E context. I don't see what value they have for normal development? Or at least, we shouldn't feel inclined to optimize for anything other than E2E tests behavior for these plugins.

[gziolo]

But, is there a reason it's done this way? Those plugins, evidenced by the path, should apply only in the E2E context. I don't see what value they have for normal development? Or at least, we shouldn't feel inclined to optimize for anything other than E2E tests behavior for these plugins.

I personally don't see any but I have no idea why it was included in the first place.

[gziolo]

Should we remove mu-plugins from https://github.com/WordPress/gutenberg/blob/master/docker-compose.yml#L16 and merge this change?

[youknowriad]

Sometimes I run tests on my actual install to debug test etc... Ideally, the plugins are enabled for the tests and disabled at the end.

[gziolo]

You can test locally using both instances created by Docker. What’s the reason you need to use the one which is dedicated to development?

[youknowriad]

No particular reason, just the simplicity of having a single instance for everything. I wouldn't mind having to create a second one but I think this should be taken into account, as other persons use vvv for development and if we force them to have plugins for testing but not for development, it will force them to have two local environments which is always harder.

[aduth]

Part of my aversion to the activate / deactivate dance is the time it adds to the build. It might be only a few seconds, but it's once on test start, another on test end † ; once for each plugin; and as of #14244, requires a pair of re-logins for the author tests.

Is it maybe possible to detect if the tests are running in a local environment and dynamically load in the mu-plugins ?

† Aside: could we consider skipping deactivation-on-end in throwaway CI environments (Travis)?

[youknowriad]

Aside: could we consider skipping deactivation-on-end in throwaway CI environments (Travis)?

I'd be fine with that, I think it's already the case for the "disable" animation plugin.

[aduth]

I'm unclear what I need to do here to move forward. Admittedly I still don't really understand why these plugins would ever be active in a developer's environment, even when using the Docker setup, unless they'd specifically use the Docker environment or plugins oriented specifically for testing.

[gziolo]

I'm unclear what I need to do here to move forward. Admittedly I still don't really understand why these plugins would ever be active in a developer's environment, even when using the Docker setup, unless they'd specifically use the Docker environment or plugins oriented specifically for testing.

I would be perfectly fine with leaving this plugin in mu-plugins folder if we remove it from the regular Docker instance:
https://github.com/WordPress/gutenberg/blob/master/docker-compose.yml#L21
and leave it as is for e2e tests setup:
https://github.com/WordPress/gutenberg/blob/master/docker-compose.yml#L77

[aduth]

I would be perfectly fine with leaving this plugin in mu-plugins folder if we remove it from the regular Docker instance:
https://github.com/WordPress/gutenberg/blob/master/docker-compose.yml#L21
and leave it as is for e2e tests setup:
https://github.com/WordPress/gutenberg/blob/master/docker-compose.yml#L77

That helps clarify, thanks.

If I understand correctly, if someone wants to have quick access to the end-to-end tests, couldn't they reference it on the wordpress_e2e_tests just as well?

I'm inclined to think the regular plugins should be removed as well, but that seems to be the point in conflict here 🤷‍♂

[youknowriad]

I can clarify what my (selfish) goals are:

• I want to be able to run npm run test-e2e file and just run the test on my dev instance, so I can easily debug,... without needing a separate environment
• Ideally, running that test disables all plugins that might have been enabled specifically for the test (at the moment the animation is kept enabled)

Does this align with your thoughts/goals?

[aduth]

• I want to be able to run npm run test-e2e file and just run the test on my dev instance, so I can easily debug,... without needing a separate environment

I can agree with the developer experience motivation here, but not with the specific implementation. Our approach to test plugins should be optimized for testing, not for regular development.

In any case, is it not a matter of just using localhost:8889 (the wordpress_e2e_tests container) instead of localhost:8888 (the wordpress container and typical development environment)?

[youknowriad]

I can agree with the developer experience motivation here, but not with the specific implementation. Our approach to test plugins should be optimized for testing, not for regular development.

Isn't "testing" part of the "developer experience" somehow 🤷‍♂

In any case, is it not a matter of just using localhost:8889 (the wordpress_e2e_tests container) instead of localhost:8888 (the wordpress container and typical development environment)

The problem of this kind of tweaks to the testing setup compared to a production setup is the fact that it makes it harder to launch tests in several different environments without having to go tweak these environments.

Say I want to run the suite on my personal blog (or on a personal test blog that is online), or on my custom WordPress Core install, I'd love to be able to just pass the URL and it works.

Right now, this means:

• Installing the test plugins
• Providing the URL

It's already a tradeoff as we need to install these plugins, but installing mu-plugins is even harder. I'd actually love to remove the first constraint :)

[aduth]

I suppose you're not referring to the specific point of Docker container configurations and what's currently assigned as volumes for the standard development environment? Otherwise, I'm not quite sure I follow.

Or is the point is that mu-plugins makes replicating tests manually more difficult? What effective difference is there between installing mu-plugins and any other plugin? How would you foresee that the first constraint would be removed?

I guess if the concern is that we shouldn't want these plugins to need to exist in the first place, I think it's fair to grant. For this one, trying to consider what it would need to take for it not to exist: I wonder why the post remains locked even after the previous session ended, considering there is logic to release the lock when the browser closes (source). I forget specifically what I'd seen since the debate here caused deadlock, but I think it may have been there was a minimum duration on the lock even if it was released. I suppose it'd be worth verifying again whether that's the case.

[youknowriad]

Or is the point is that mu-plugins makes replicating tests manually more difficult? What effective difference is there between installing mu-plugins and any other plugin? How would you foresee that the first constraint would be removed?

You're right that you could install these mu-plugins as regular plugins in an environment you want to test and enable those. I'd like us to document what it takes to run these tests in environments outside the Gutenberg repo. How do I run these tests on my vvv WordPress trunk environment. (Even if it includes manual steps).

I think I've come to be more onboard with the idea 🤷‍♂

About the post locking

maybe it was just a temporary issue due to the fact that we relied on a deprecated browser feature at some point and since fixed I believe.

[aduth]

Trying to find a way forward here: #22256 (comment)

Worst case, I think it could be fine to refactor this as a regular plugins/ plugin activated by default in end-to-end tests.