Fix failing / flaky Navigation block e2e test by removing unnecessary validation steps for creation of draft page #37755
Conversation
|
Size Change: +3.66 kB (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
getdave
changed the title
|
Failed once but because of the different Navigation test: |
|
I don't think the fix in #37501 is correct. Using a mock in this way effectively changes behavior of the application and makes this test have less value. The test is no longer testing what happens in the real world. The request does 404 reliably in production, so I think the test should expect that to happen rather than change the way the endpoint works to return an empty array. I'm wondering why |
I'm not sure about that one. The test is failing because of a 404 _in the consol_e. That isn't an error with the Gutenberg application code. Rather it's a network issue. Yes this 404's in production but that's ok because that is handled gracefully in the UI. Check it in production and you'll see that happening. All we're going is mocking an endpoint that's largely irrelevant to this test to avoid an unreliable 404 in the console causing a test failure. Bottom line - network 404 != bug in application code. |
|
Also worth noting that with this fix (and the other one) in place we now have had this flaky test pass ✅ 5 times. |
Yes, that's exactly my point. It should be handled gracefully in the UI and we should have tests in place to assert that this works. But we've now lost test coverage of this, because the endpoint had been modified to return an array in the end to end test. |
I understand but...this test isn't asserting on handling the rich previews. It's purely dealing with page creation in the Nav block. My thought was that we should have another test that explicitly checks for graceful handling of rich previews. We're only mocking what is not directly under test.
I think if we're resorting to adding workarounds to wait for network conditions then we're probably not constructing the test correctly. Network condition variability on CI is very often what causes flaky tests. ...but maybe that isn't working. Likely because the rich previews API will only 404 once the response comes back from the network. We were expecting a 404 in the test which only sometimes occurred because the rich preview endpoint is not what is under test. |
This is true, though it is unfortunately part of the flow being tested, so it makes sense to write the test correctly. The change still goes against the purpose of response mocking, which should be to provide realistic data. Returning an empty array here is modifying the runtime behavior of Gutenberg from testing code.
I don't really see it as a workaround. We need to wait for the 404 response to happen before asserting that it happens, |
|
Thanks for the feedback and discussion. It's good to challenge my assumptions here. As this is failing multiple PRs I'd like to move towards a solution. My question now is: what would you like to see changed for you to feel comfortable with this PR? Is it:
OR
I'd vote for |
|
I agree with @talldan here that we shouldn't use unrealistic mock data and that The ideal solution would either be:
As for now, if this is blocking us, I agree to merge this as is (maybe with comments) to unblock us, but also create an issue to track this. |
|
One thing I just realised is that this test is skipped so it's less urgent than I thought 😄 Still...it's good to not have disabled tests for too long. |
|
@talldan Now using appropriate mock data here. |
|
I still don't think that's right. The endpoint would normally return a payload like that, and the test doesn't ascertain that the front-end code handles the 404 from url-details. I think it'd be good to try making the test work properly without injecting incorrect data. I had a go in #37901, it passed consistently locally, so hopefully it does the same on CI 🤞 . BTW - has there been a discussion about whether a 404 is the correct thing for url-details to return? My take is that it seems incorrect. The That the server couldn't find any details about the url is unrelated, that should be relayed via the response payload. |
|
Happy enough to close this in favour of #37901. |
Description
The test
allows pages to be created from the navigation block and their links added to menufrom the Navigation block e2e tests is (still) intermittently failing.We tried to fix this in #37501 but it appears that was only half the battle!
Running locally it appears we've got some additional steps which attempt to validate that a draft page has been created that might not be necessary.
I believe the behaviour of the Nav block no longer requires that we reselect the Nav item in order to show the link preview.
I've removed that step and it seems to pass.
Fixes #37479
How has this been tested?
We need to re-run CI several times (let's say x5) and check that the test is no longer flaky.
Please feel free to mark test runs below:
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).