-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 E2E tests for shared blocks #7947
Conversation
Tests the following shared block flows: - Converting a block to a shared block, naming it, saving - Converting a block to a shared block, saving - Inserting a shared block, renaming it, editing it, saving - Converting a shared block to a regular block - Deleting a shared block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - I'm concerned by the overall maintainability of our e2e tests, particularly with the selector/xpath names. As the tests grow, it'll become a bit of a nightmare if we want to change some copy or a class name. I can make a separate issue for that, but this looks good to go.
|
||
// Convert block to a shared block | ||
await page.click( 'button[aria-label="More Options"]' ); | ||
const convertButton = await page.waitForXPath( '//button[text()="Convert to Shared Block"]' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Selecting elements looks like fun - have we considered using data attributes on our elements to make writing tests a bit easier? e.g. data-test-name: "convert-to-shared-block-button"
. We could also make test objects for the components containing the selectors so that tests are easier to write const convertButton = await page.waitForXPath( convertToSharedBlockButton.root );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made an issue here:
#7974
it( 'can be inserted and edited', async () => { | ||
// Insert the shared block we created above | ||
await insertBlock( 'Greeting block' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should ideally be no concept of "above" as assuming tests to be run in particular order (or even that any others are run at all). I should be able to change this to it.only
and the test should be fine to run in isolation. Common behaviors should be established in lifecycle (beforeEach
or beforeAll
) or repeated in every test case.
Note: I'm encountering this in implementing a new end-to-end test case for verifying the fix at #21205. Ideally I'd like to iterate on the test case with it.only
, but at least as currently presented, there's no prior art for me to leverage commonly available reusable blocks to work with. For the purposes of my test, I might just copy the logic from "can be created" to create the reusable block. It could be straight-forward enough to refactor to its own function; not sure if I'll do that or end up just following the pattern here of reusing the block created in an earlier test case once finished.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right, the setup code belongs in a beforeAll()
. I think this might have been the first E2E test I ever wrote. Ah, to be young...
I created #21249 to track this.
Closes #7880.
Adds E2E tests for the following flows:
These were particularly finicky to write as there's lots of asynchronous things happening. Let's see what Travis CI says! 🤞