chore: Isolate e2e tests and reset them for each run #8041

tofumatt · 2018-07-19T00:08:52Z

This is a start to isolating our end-to-end (e2e) tests from our development environment so that tests are not affected by what's going on in the development environment.

This came about because I was writing e2e tests for #7941, but the existing comments elsewhere on the development site were causing a mismatch with the number of comments I expected in the development environment.

This is only a start because there are things that aren't cleared by the current afterEach code run after every test, but we can add to that as we go. The aim here is that every time we start e2e tests we should have a blank slate and after every test run we should also have a blank slate, without any requirements on developers to clean up the remnants of their tests.

Fixes #7959
Fixes #8010

Testing Instructions

Run ./bin/setup-local-env.sh and make sure there are no error.
Run npm run test-e2e and see that the tests pass and do not create any data in your development site's database

tofumatt · 2018-07-19T00:09:55Z

bin/reset-e2e-tests.sh

+. "$(dirname "$0")/includes.sh"
+
+# Set up WordPress site used for end-to-end (e2e) tests.
+. "$(dirname "$0")/install-wordpress.sh" --e2e_tests


If we can slim this down to just deleting posts and such I'm game, but @aduth mentioned in #8010 we might want to uninstall plugins and such. At that point it seemed easier to just re-install at the outset of every test run.

A reinstall is pretty quick, and it guarantees you're getting a fresh state.

tofumatt · 2018-07-19T00:11:16Z

docker-compose.yml

+      - ./test/e2e/test-plugins:/var/www/html/wp-content/plugins/gutenberg-test-plugins
+      - ./test/e2e/test-mu-plugins:/var/www/html/wp-content/mu-plugins
+
+  mysql_e2e_test:


This seems like overkill, but I couldn't find a(n easy) way to create two separate databases we could use in the same container from different WordPress images. I'd like to just have a wordpress and wordpress_e2e_test database in the same container, so if we can easily do that, let me know.

tofumatt · 2018-07-19T00:11:58Z

test/e2e/support/bootstrap.js


 // The Jest timeout is increased because these tests are a bit slow
-jest.setTimeout( 100000 );
+jest.setTimeout( PUPPETEER_TIMEOUT || 100000 );


This seemed a handy thing to have so I added it, but if it's no good let me know.

tofumatt · 2018-07-19T00:13:18Z

test/e2e/support/bootstrap.js

+
+	await visitAdmin( 'edit.php' );
+
+	const bulkSelector = await page.$( '#bulk-action-selector-top' );


I'm thinking I should add some comments to this explaining what it does, but the gist is that it opens the admin page and deletes every post on the page. This clears all posts and comments created by tests.

In theory, if we made so many posts in a single test that it spanned multiple pages we'd need to adjust this, but I started simple for now as that never happens.

I'm wondering if we shouldn't perform it as beforeAll to support also the case where the previous run was interrupted by some random factor and cleanup could NOT be finished.

Well, the first time the tests are run the entire setup is reset: https://github.com/WordPress/gutenberg/pull/8041/files#diff-cc6d2a226eaf023f937fb5813d954327R10

I guess doing it in either place is fine though, so we can change it to beforeEach if you like. Actually, doing that might save us accepting the "Do you want to leave this page?" dialog so I'll give it a go 👍

Just realised that global.page doesn't exist in the beforeAll.

We should really nix the globals in the test suite as well, they make it weird to reason about the state of tests and I don't think they're useful; but that's a separate issue I'll get into after this is fixed 😄

tofumatt · 2018-07-19T00:18:57Z

bin/install-wordpress.sh

+# Gutenberg script includes.
+. "$(dirname "$0")/includes.sh"
+
+CLI='cli'


This is all a bit messy, I might clean it up later 😅

pento

I'm generally cool with this reorganisation, I just have a few comments. Nice work @tofumatt. 🙂

pento · 2018-07-20T05:29:43Z

bin/install-wordpress.sh

+	docker-compose run --rm $CLI option update siteurl "http://localhost:$HOST_PORT" >/dev/null
+fi
+
+# Reset the database so no posts/comments dirty up tests, if any exist.


Probably only want to do this if --e2e_tests is defined.

Yeah, fair enough. I like having mine reset but it would be unexpected. Will change 👍

pento · 2018-07-20T05:30:15Z

bin/reset-e2e-tests.sh

+. "$(dirname "$0")/includes.sh"
+
+# Set up WordPress site used for end-to-end (e2e) tests.
+. "$(dirname "$0")/install-wordpress.sh" --e2e_tests


A reinstall is pretty quick, and it guarantees you're getting a fresh state.

pento · 2018-07-20T05:37:00Z

docker-compose.yml

+      - ./test/e2e/test-plugins:/var/www/html/wp-content/plugins/gutenberg-test-plugins
+      - ./test/e2e/test-mu-plugins:/var/www/html/wp-content/mu-plugins
+
+  mysql_e2e_tests:


I'm as excited as anyone about containers, but do we really need a seperate MySQL container here? Does it provide any isolation benefit that using a different DB_NAME doesn't give?

Is that a lot of work (mostly time wise) to create a new database per one tests run when booting tests?

Or use some WP-CLI incantation to restore it to a fresh install? In general, we are using WP Admin UI to do some things which WP-CLI, in theory, could do with one line, like activating/disabling plugins. I have no idea if it is easy to integrate, but I thought I ask :)

It takes a few seconds to drop the database and re-install WordPress, I think the slowest part is Docker taking a snapshot of the cli box when the command runs.

I'm more concerned about having two MySQL servers running at the same, chewing up RAM.

I agree.

I thought I mentioned it... maybe it's removed in a new commit, but yeah I would prefer a single DB too. I think the Docker container itself couldn't do one, but we should be able to easily create a new database used for the test runs on the existing DB/container.

I'll change this up to use the existing DB. I wanted to use the same WP-CLI container as well but we'd need to install stuff in WordPress to make RESTful WP-CLI commands work if I understand it correctly and the CLI container is lightweight enough. 🤷‍♂️

gziolo · 2018-07-20T06:24:20Z

Yes, agreed. The general direction of having isolated DB instance in a clean state for every full tests run is great. I'd love to see it in master really soon.

tofumatt · 2018-07-20T09:46:31Z

I should also mention I considered using multisite to create a separate test site in the same container, but:

It actually seemed more complicated
It means our WordPress install is "less stock"
It means actually testing multisite can be its own, isolated thing in the test container

Just in case anyone mentions that as an alternative. I suppose it is but I think this is better.

Also, @gziolo and I were chatting and he mentioned we could, maybe in the future, actually have a multisite where each test ran on a site, to up concurrency. Who knows if that'd work but it sounds cool/fast. 😮

Fixes #7959 Fixes #8010

tofumatt · 2018-07-21T01:04:10Z

Improvements made, including omitting the second MySQL container and only resetting the database for the test site.

pento

Noice! I like this change. 🙂

tofumatt · 2018-07-26T10:03:13Z

I just realised: I think I should add a notice to users who will encounter the need to re-run ./bin/setup-local-env.sh after this merges when they run tests with `npm run test-e2e.

I'll add a little script to check for it... if it's complicated and requires further review I'll ping someone for a quick look 🙂

youknowriad · 2018-07-27T10:42:16Z

I noticed an increase in test instability recently and also the duration of the e2e tests increased significantly, I suspect it's coming from this PR. Do you think we can try to revert it and see if it's better to give us a bit more time to polish it?

tofumatt · 2018-07-27T10:47:40Z

I could imagine slower tests, but what kind of instability are you seeing? If it’s causing really bad issues we should revert, but it’s a blocker to writing better, reliable tests 😊 - Matt (Sent from mobile)

…

On 27 Jul 2018, at 11:42, Riad Benguella ***@***.***> wrote: I noticed an increase in test instability recently and also the duration of the e2e tests increased significantly, I suspect it's coming from this PR. Do you think we can try to revert it, see if it's better to give us a bit more time to polish it? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

youknowriad · 2018-07-27T13:20:17Z

A lot of "timeout" issues I didn't see before, I can't get the tests to pass reliably in one of my PRs. I agree with the premises of the PRs: more reliable and stable tests but it's just that it's less stable for now. I might try a revert PR to see if it's better and we can see how we can improve this later without introducing more instability.

This reverts commit 1a0b22a.

youknowriad · 2018-07-27T14:21:13Z

After more tests, I noticed that clearing the travis cache can make things better. I'm not certain yet if the failures are equivalent to what we had before this PR. I'm not reverting at the moment, but we should definitely work on stabilizing those tests.

tofumatt requested a review from a team July 19, 2018 00:08

tofumatt commented Jul 19, 2018

View reviewed changes

tofumatt changed the title ~~chore: Isolate e2e tests and reset themn for each run~~ chore: Isolate e2e tests and reset them for each run Jul 19, 2018

tofumatt commented Jul 19, 2018

View reviewed changes

tofumatt mentioned this pull request Jul 19, 2018

Add latest comments block #7941

Merged

3 tasks

gziolo requested review from pento and ntwb July 20, 2018 05:25

gziolo added [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Build Tooling Issues or PRs related to build tooling labels Jul 20, 2018

pento reviewed Jul 20, 2018

View reviewed changes

tofumatt added 4 commits July 20, 2018 23:49

chore: Isolate e2e tests and reset themn for each run

946ad7c

Fixes #7959 Fixes #8010

Tidy up code and do not install test site on load

25c64fc

docs: Improve comments

76419de

chore: Use fewer containers, better scripts

313c5d7

tofumatt requested a review from a team July 21, 2018 01:03

tofumatt force-pushed the try/7959-isolate-e2e-tests-8010-reset-tests-at-start branch from b8fe2da to 313c5d7 Compare July 21, 2018 01:03

tofumatt requested a review from pento July 25, 2018 10:41

pento approved these changes Jul 26, 2018

View reviewed changes

chore: Inform users of new tests

4e91c77

tofumatt merged commit 1a0b22a into master Jul 26, 2018

tofumatt deleted the try/7959-isolate-e2e-tests-8010-reset-tests-at-start branch July 26, 2018 16:08

youknowriad added a commit that referenced this pull request Jul 27, 2018

Revert "chore: Isolate e2e tests and reset them for each run (#8041)"

5dff2c2

This reverts commit 1a0b22a.

youknowriad mentioned this pull request Jul 27, 2018

Revert "chore: Isolate e2e tests and reset them for each run (#8041)" #8256

Closed

tofumatt mentioned this pull request Jul 31, 2018

Allow tests to run against non-Docker environments again #8317

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Isolate e2e tests and reset them for each run #8041

chore: Isolate e2e tests and reset them for each run #8041

tofumatt commented Jul 19, 2018 •

edited

Loading

tofumatt Jul 19, 2018

pento Jul 20, 2018

tofumatt Jul 19, 2018

tofumatt Jul 19, 2018

tofumatt Jul 19, 2018

gziolo Jul 20, 2018 •

edited

Loading

tofumatt Jul 20, 2018

tofumatt Jul 21, 2018

tofumatt Jul 19, 2018

pento left a comment

pento Jul 20, 2018

tofumatt Jul 20, 2018

pento Jul 20, 2018

pento Jul 20, 2018

gziolo Jul 20, 2018

pento Jul 20, 2018

tofumatt Jul 20, 2018

gziolo commented Jul 20, 2018

tofumatt commented Jul 20, 2018 •

edited

Loading

tofumatt commented Jul 21, 2018

pento left a comment

tofumatt commented Jul 26, 2018

youknowriad commented Jul 27, 2018 •

edited

Loading

tofumatt commented Jul 27, 2018 via email

youknowriad commented Jul 27, 2018

youknowriad commented Jul 27, 2018


		await visitAdmin( 'edit.php' );

		const bulkSelector = await page.$( '#bulk-action-selector-top' );

chore: Isolate e2e tests and reset them for each run #8041

chore: Isolate e2e tests and reset them for each run #8041

Conversation

tofumatt commented Jul 19, 2018 • edited Loading

Testing Instructions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pento left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Jul 20, 2018

tofumatt commented Jul 20, 2018 • edited Loading

tofumatt commented Jul 21, 2018

pento left a comment

Choose a reason for hiding this comment

tofumatt commented Jul 26, 2018

youknowriad commented Jul 27, 2018 • edited Loading

tofumatt commented Jul 27, 2018 via email

youknowriad commented Jul 27, 2018

youknowriad commented Jul 27, 2018

tofumatt commented Jul 19, 2018 •

edited

Loading

gziolo Jul 20, 2018 •

edited

Loading

tofumatt commented Jul 20, 2018 •

edited

Loading

youknowriad commented Jul 27, 2018 •

edited

Loading