Image block: Fix flaky tests #52442
Image block: Fix flaky tests #52442
Conversation
|
I want to try and run these tests a few times here on Github to see if the flakiness gets resolved. If anyone has any suggestions on how else I can attempt to address this, please let me know! 🙏 |
|
Size Change: +3.01 kB (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
|
@Mamaduka I created this pull request based on your comment on #51305. I've so far been unable to reproduce the unreliability of the tests locally, so the fixes I've pushed here are a guess as to what might be causing the issue based on where the tests have been failing. Two questions: |
There was a problem hiding this comment.
@artemiomorales, when you check the logs in flaky test issues, you'll see that most are failing with the following error:
Expected pattern: /b3dbc133-fbc9-417f-abd4-5aa4f9974d9e/
Received string: "blob:http://localhost:8889/f4e413c1-7eed-403f-b80d-a48f50798650"
This means the image URL is a blob instead of a string. The image block will display a blob while an image is uploading to show immediate feedback.
I usually use traces from test failure artifacts for debugging the e2e test.
See the Playwright docs for more details - https://playwright.dev/docs/trace-viewer.
There was a problem hiding this comment.
@artemiomorales, do you mind rebasing the branch on top of the trunk? Then we can land this after all checks are green.
artemiomorales
force-pushed
the
fix/image-block-flaky-tests
branch
from
ac2b74d
to
1104190
Compare
|
@Mamaduka Okay so the good news is I've managed to reproduce this locally but it seems this PR as is won't address the issue. Like you said, it looks like the tests are failing because it's taking too long to upload the responsive image. Locally, I've been testing using a throttled network and have removed the use of the large responsive image wherever it isn't being explicitly used. It's still present in two tests though and am not quite sure how to resolve yet, as increasing the test timeout seems to have no effect. Will keep hammering at this, though am happy to hear ideas! |
|
@artemiomorales are we sure it's taking too long to upload and it isn't that we're running through test code before we give the editor time to update? after image upload, the image block has to dispatch some actions to update its attributes, which is what we're seeing with the blobs. I don't see anywhere that we're explicitly running all queued tasks and microtasks which would give space for those dispatches to resolve, if they need to. you might try wrapping a I'm worried that if we try to solve this by using a smaller image we're going to be chasing the problem every so often because we've left in the race condition and simply tried to get ahead of it instead of eliminating the race which is at the heart of the flakiness. another potential option to try is to manually dispatch some action in the test code after update, or even inside that timeout. // Give the image block the chance to update after the upload is finished.
await new Promise( resolve => {
setImmediate( async () => {
const image = imageBlock.locator( 'role=img' );
await expect( image ).toBeVisibile();
await expect( image ).toHaveAttribute( … );
} );
} );
await editor.openDocumentSettingsSidebar(); |
|
Maybe we should skip these tests while trying to find a stable solution for the flakiness. They fail often. |
IIRC, the image block sets a temporary blob url before the upload finishes. This means that when the test timeouts (5s) the image still has the temporary url. It's possible that the upload is taking too long for the test. Maybe we should just try increasing the timeout for that action. await expect( image ).toHaveAttribute(
'src',
- new RegExp( filename )
+ new RegExp( filename ),
+ { timeout: 10_000 }
);
Playwright automatically waits and retries until the assertion passes. So, yes, we're waiting, just implicitly, matching what end users do. IMHO we shouldn't rely on implementation details in e2e tests. Instead, we should wait, retry, and fail with a reasonable timeout. |
|
The image block wrapper receives the gutenberg/packages/block-library/src/image/edit.js Lines 312 to 313 in 2704078 |
@Mamaduka Sounds good! I've created a PR that disables the tests for now. |
This removes the uploading of a large image from responsive tests that do not explicitly need it. For the tests that do need the image, it increases the timeout to give enough time for the image to finish uploading.
@dmsnell @kevin940726 Thanks for taking a look at this! I've increased the timeout for the assertion on the image The tests passed — let's merge and give this a go. |
can we also follow-up here and try to remove the race condition? or at a minimum add a note or a |
priethor
added
[Type] Automated Testing
|
@dmsnell I don't think there's any race condition here as mentioned in #52442 (comment). Or am I misunderstanding or missing something? |
|
I don't think we have to move the logic out of
I believe that's because these two timeouts serve different purposes. The global timeout is the maximum duration for a test but it doesn't change the assertion/action timeout. |
|
@kevin940726 I meant mainly that increasing the timeout acknowledges that the test passing depends on something that takes different amounts of time on different computers or situations. given that we don't expect the image upload to fail, it seems like we should be able to rely on discrete circumstances in some way. or maybe our timeout ought to be something ridiculous, like a minute or five minutes or an hour - something that is clearly never going to be an issue. for instance, could we assert that the blob is there first, and then wait again for the expected URL to appear? why is the upload time varying enough that it causes the tests to be flakey. that's the inherent race. maybe it's not possible to do what I want, but expanding the timeout marginally seems to be making a flakey test less flakey, but leaving it inherently flakey. |
|
I understand, but uploading is a task that doesn't have a deterministic waiting time. The best we could do is to wait for the end result. Oftentimes, the default action timeout (5s) makes sense as what end users would wait until they think something went wrong. The problem isn't that we're using the waiting technique, it's that uploading takes too long. I don't think uploading a 50KB image should take more than 5 seconds. Perhaps there's something unexpected happening in the backend and we should fix that instead. For now, increasing that one action's timeout fixes the cause of the flakiness but doesn't fix the "bug" we have in the app. |
I agree that this was a good change to clear up the failing tests; I just wanted to see if we could find the cause of the problem instead of finishing the work here. Maybe there's no solution 🤷♂️ |
|
I agreed! I believe the cause of the problem is in the backend and we probably can't fix it here. Good to keep in mind in case of a future refactor of the media library though! |
|
when you say "backend" what are you talking about? isn't this all running within Playwright/Chromium? |
|
I mean the PHP side, where we process the uploads. Playwright merely operates the browser, it has no controls over the backend. This is what I'm seeing when manually uploading the same image in chrome devtools:
It took 3 seconds for the server to process the 50KB image. It's fair to assume it might take longer on a resource-limited machine like in the CI environment. I'm guessing it might depend on the size and resolution of the image that makes it take so long to process. |
|
hm. this is interesting @kevin940726, thanks for clarifying. though Playwright doesn't have control over the PHP, I guess we do. I wonder if we could modify the WP environment for these tests to return earlier so that the delay goes away. this fundamental problem, of the PHP taking a while to respond, is there for all our tests that involve exchanges with the server. on the other hand, does this test even rely on getting the original filename back into the image source? what if we merely tested that the image is created with a |
Yep! But I think the code is in Core though and probably limited here.
Exactly. That's why I proposed switching to In theory, we can upload it once in a gutenberg/test/e2e/specs/editor/plugins/image-size.spec.js Lines 35 to 45 in 6e4bc84 |
|
sounds like a good idea @kevin940726 - thanks for suggesting it and providing an example. |
mikachan
added
[Feature] Blocks
What?
The image tests have recently been flagged as flaky. This PR attempts to address that.
Why?
We want our tests run efficiently and reliably.
How?
I believe this issue may be caused by inconsistent uploading behavior in the
beforeEach(). I've moved the upload logic outside of that call and instead do it manually for most of the tests now.Testing Instructions
Unfortunately, I'm not sure it's possible to test this locally. You can try running
npm run test:e2e:playwright -- image.spec.jsto make sure the tests pass, but the flakiness appears to only be present on Github.Testing Instructions for Keyboard
N/A
Screenshots or screencast
N/A