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

Add basic E2E test for blocks. #174

Merged
merged 29 commits into from
Mar 9, 2023
Merged

Add basic E2E test for blocks. #174

merged 29 commits into from
Mar 9, 2023

Conversation

ryanwelcher
Copy link
Contributor

Introduces basic e2e test to confirm that each block is inserted in a new post.

Closes #173.

@ryanwelcher ryanwelcher marked this pull request as ready for review October 25, 2021 17:48
@ryanwelcher ryanwelcher requested a review from mkaz October 25, 2021 18:15
@ryanwelcher
Copy link
Contributor Author

I've got the tests running locally but am intermittently running into timeout errors and the workflow is having issues as well. Open to any suggestions here.

@mkaz
Copy link
Member

mkaz commented Oct 26, 2021

🤔 I think it might be overkill to have e2e tests on example repo. The issue we have with the examples is we don't keep them in-sync with Gutenberg changes. Having an e2e tests won't necessarily help because:

  1. the tests only run when triggered, so if no changes are made, like the last gap, nothing will trigger the test,

  2. backward compatibility in Gutenberg means all the examples would still work, and tests would pass, even if they are severely outdated as they were.

So, the case they are testing for is the basic case that the examples work, and that should be covered by reviews merging in the PR.

It might be interesting to create an example block and showing how to create an e2e test for that block.

// Check if block was inserted
expect( await page.$( `[data-type="${ name }"]` ) ).not.toBeNull();

expect( await getEditedPostContent() ).toMatchSnapshot();
Copy link
Member

Choose a reason for hiding this comment

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

The snapshots generated are very simple so we can inline them into the test as explained in https://jestjs.io/docs/snapshot-testing#inline-snapshots. It would make tests easier to follow and it should work exactly the same.

@gziolo
Copy link
Member

gziolo commented Oct 26, 2021

I've got the tests running locally but am intermittently running into timeout errors and the workflow is having issues as well. Open to any suggestions here.

Error: thrown: "Exceeded timeout of 5000 ms for a test.

It seems like 5 seconds for the e2e test is not enough. There is a different limit set in Gutenberg:

https://github.com/WordPress/gutenberg/blob/9a58337f7e04e95fe54f636fbddf62821d4d79ce/packages/e2e-tests/config/setup-test-framework.js#L68-L69

We will have to find a reasonable duration here as well. Ideally, we should increase the timeout in the default config for e2e tests in @wordpress/scripts. As a temporary workaround, we can set it through process.env.PUPPETEER_TIMEOUT in package.json or set manually in each test.

🤔 I think it might be overkill to have e2e tests on example repo. The issue we have with the examples is we don't keep them in-sync with Gutenberg changes. Having an e2e tests won't necessarily help because

There are two goals of having e2e tests here:

  • it shows how to write tests and configure the environment
  • it will help us catch some simple errors while updating the block code in the future

You made a good point that we won't have a feedback loop when something impactful would land in the Gutenberg repository. However, we can assume that there should never be breaking changes in WordPress and even when that happens we should be able to catch that next time we land a PR in this repository.

@ryanwelcher
Copy link
Contributor Author

ryanwelcher commented Oct 26, 2021

🤔 I think it might be overkill to have e2e tests on example repo. The issue we have with the examples is we don't keep them in-sync with Gutenberg changes. Having an e2e tests won't necessarily help because:

I tend to agree with @gziolo that these tests are more about teaching than catching errors. Although, the "does it insert" test would catch the most basic issues/regressions.

It might be interesting to create an example block and showing how to create an e2e test for that block.

This is an excellent idea. I wonder if instead of a single folder for all the tests, we add one for each block to follow how Gutenberg approaches adding tests. We can have basic test in there i.e basic.spec.js then add a new test for each feature that the example blocks are introducing.

@gziolo
Copy link
Member

gziolo commented Oct 26, 2021

It might be interesting to create an example block and showing how to create an e2e test for that block.

This is an excellent idea. I wonder if instead of a single folder for all the tests, we add one for each block to follow how Gutenberg approaches adding tests. We have had the basic test in there i.e basic.spec.js then add a new test for each feature that the example blocks are introducing.

Good thinking. As far as I remember, as long as the test ends with *.spec.js it should be detected by Jest runner 👍🏻

@mkaz
Copy link
Member

mkaz commented Oct 26, 2021

I would prefer not to add one to each of the blocks, especially the early basic blocks because it muddies what the person is trying to learn. The initial goal is just to get the very basics down of what is a block, how do I even create one.

If there is a lot of additional tests and other pieces that aren't core to those ideas it gets confusing.

I like a more progressive approach, so maybe we have some "complete blocks" that show all the pieces together including an e2e tests, but including them too early upfront will just make it all seem overly complicated and too much stuff to learn at once.

@ryanwelcher
Copy link
Contributor Author

I like a more progressive approach, so maybe we have some "complete blocks" that show all the pieces together including an e2e tests, but including them too early upfront will just make it all seem overly complicated and too much stuff to learn at once.

This is a very good point, perhaps we can exclude tests for Basic, Basic ( ESNext ) and Stylesheets? Those are very simple blocks and don't really benefit much from the tests. I'd say that we still need an example of how to test that a block is inserted, but we can start them with Editable. Thoughts?

@gziolo
Copy link
Member

gziolo commented Oct 27, 2021

I opened WordPress/gutenberg#35983 to address the issue with the default timeout.

@ryanwelcher
Copy link
Contributor Author

@gziolo with WordPress/gutenberg#35983 merged, maybe we should wait for the next version of @wordpress/scripts to go out and I can remove the timeout entries here.

@mkaz any thoughts on my last suggestion of removing the tests from the first few examples?

@ryanwelcher
Copy link
Contributor Author

Thanks for confirming! I'll update this PR and get it approved.

package.json Outdated Show resolved Hide resolved
@ryanwelcher ryanwelcher requested a review from gziolo March 7, 2023 19:46
@ryanwelcher
Copy link
Contributor Author

@gziolo I've made some updates to the tests to get them running. These are very basic but are a good starting point.

@ryanwelcher ryanwelcher mentioned this pull request Mar 7, 2023
5 tasks
Copy link
Member

@ntwb ntwb left a comment

Choose a reason for hiding this comment

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

Looks great @ryanwelcher, I added some build tweaks, changes and suggestions, otherwise looks good to merge and iterate on

.github/workflows/e2e-tests.yml Outdated Show resolved Hide resolved
.github/workflows/e2e-tests.yml Outdated Show resolved Hide resolved
.github/workflows/e2e-tests.yml Show resolved Hide resolved
.eslintignore Outdated Show resolved Hide resolved
@ryanwelcher
Copy link
Contributor Author

@ntwb thanks for the review! I'll get these changes in today!

ryanwelcher and others added 4 commits March 8, 2023 12:13
Co-authored-by: Stephen Edgar <stephen@netweb.com.au>
Co-authored-by: Stephen Edgar <stephen@netweb.com.au>
@ryanwelcher ryanwelcher requested a review from ntwb March 8, 2023 18:02
@ryanwelcher
Copy link
Contributor Author

@ntwb changes implemented. Mind re-reviewing and approving?

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Looks like a great start 👍🏻

Some notes:

  • e2e tests will run twice on different Node.js version, we should monitor it as technically it should be enough to run with Node LTS (18.x as of today)
  • I think it's more important to run static checks (lining, build, etc) with all supported Node versions (14, 16, 18 as of today - 20 will replace 14 at the end of April)
  • this PR removes the unit tests that were included as an example, but I don't think we had CI integration that executed them

Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>
@ryanwelcher
Copy link
Contributor Author

  • e2e tests will run twice on different Node.js version, we should monitor it as technically it should be enough to run with Node LTS (18.x as of today)

Good to know.

  • I think it's more important to run static checks (lining, build, etc) with all supported Node versions (14, 16, 18 as of today - 20 will replace 14 at the end of April)

I've committed your suggestion here.

  • this PR removes the unit tests that were included as an example, but I don't think we had CI integration that executed them

I removed them because the were broken and failed. I'd love to add unit tests back in but I think we need practical working examples.

@ryanwelcher ryanwelcher merged commit 64cfa69 into trunk Mar 9, 2023
@ryanwelcher ryanwelcher deleted the feature/basic-e2e-tests branch March 9, 2023 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating e2e Test
4 participants