Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upFix intermittent transform e2e test failures #18662
Conversation
beforeEach( async () => { | ||
await setPostContent( '' ); | ||
await page.click( '.editor-post-title .editor-post-title__block' ); | ||
await createNewPost(); |
This comment has been minimized.
This comment has been minimized.
gziolo
Nov 21, 2019
Member
It might slow down the execution of those tests. It needs to be confirmed whether the total execution time on Travis doesn't increase.
This comment has been minimized.
This comment has been minimized.
ellatrix
Nov 21, 2019
Author
Member
It will be slower for sure. But slower is better than failing all the time?
This comment has been minimized.
This comment has been minimized.
ellatrix
Nov 21, 2019
Author
Member
Most other tests start from a new post as well, that's nothing new.
This comment has been minimized.
This comment has been minimized.
aduth
Nov 21, 2019
Member
I agree, it may be slower than it was previously, but this is a pattern we adopt in many other tests.
I would actually very much like to see an improvement to the implementation of createNewPost
where we try to artificially reset the state of the editor to an initial state, rather than perform a full reload. But that can be considered a separate task.
This comment has been minimized.
This comment has been minimized.
gziolo
Nov 21, 2019
Member
This one is quite unique, this test suite alone runs for 5 minutes on Travis.
This comment has been minimized.
This comment has been minimized.
jorgefilipecosta
Nov 25, 2019
Member
I think these tests, proved useful and caught some bugs with transforms in the past. But now transforms are mostly stable and rarely change and given the complications, they are giving us I guess we can remove them and then include some specific tests for some transforms.
This comment has been minimized.
This comment has been minimized.
jorgefilipecosta
Nov 25, 2019
Member
@ellatrix, would you prefer to update this PR to remove the tests instead, or I can submit a different PR that removes them?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
gziolo
Nov 26, 2019
Member
But now transforms are mostly stable and rarely change and given the complications, they are giving us I guess we can remove them and then include some specific tests for some transforms.
Yes, we definitely want to keep them but for a limited set of blocks
@jorgefilipecosta thinking about it a bit more, would it be possible to recreate this test suite by creating 3 sections (text, media, group) and listing fixtures for the source of blocks and run checks on the expected snapshots?
const insertButton = ( await page.$x( | ||
`//button//span[contains(text(), '${ name }')]` | ||
) )[ 0 ]; | ||
await insertButton.evaluate( ( element ) => element.scrollIntoView() ); |
This comment has been minimized.
This comment has been minimized.
aduth
Nov 25, 2019
•
Member
It's a bit surprising to me that it's necessary, given that the default implementation of click
should be doing the scrolling already on our behalf:
I wonder if it has to do with the fact that the element is in it's own scrollable region, not the page's default scroll area. If that's the case, seems like an area of improvement for Puppeteer to consider.
Edit: Related:
This comment has been minimized.
This comment has been minimized.
ellatrix
Nov 25, 2019
Author
Member
Yeah, I stumbled upon the same code and the same issues. It's not exactly the same as element.scrollIntoView()
will force the element to the top even if it's already in view.
LGTM |
This comment has been minimized.
This comment has been minimized.
Noting that I'm still observing an intermittent failure similar to what was intended to be addressed here.
At #18753, after a rebase to include revisions from this pull request. |
This comment has been minimized.
This comment has been minimized.
@aduth Yeah that's odd. Not sure what is causing it. I'm also not immediately sure how to debug. |
ellatrix commentedNov 21, 2019
•
edited
Description
Creating a new post for every this seems to resolve this. All other tests start from a new post as well. I restarted the build a few times and the transform e2e tests didn't fail. There's some other intermittent failures that should be resolved separately.
How has this been tested?
Screenshots
Types of changes
Checklist: