Navigation block e2e tests: default to my most recently created menu #48132
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +709 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in e0e8910. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4372454723
|
MaggieCabrera
force-pushed
the
e2e-navigation-default-recent
branch
from
733f538 to
0463f38
Compare
MaggieCabrera
commented
Mar 8, 2023
| } ); | ||
|
|
||
| // The two menus are created with the same timestamp, so we need to wait a bit to ensure the second one is created after the first one. | ||
| await editor.page.waitForTimeout( 2000 ); |
There was a problem hiding this comment.
In my environment both menus were being created with the same timestamp (same second) and when being evaluated to check for the newest one, the older one was the one being returned.
There was a problem hiding this comment.
Could you elaborate a bit more on why this is needed? Is this a limitation of the REST API?
There was a problem hiding this comment.
I was looking for the code to show you why I did this and it turns out yesterday it was refactored and this is not needed anymore, so I removed it!
There was a problem hiding this comment.
It looks like the function that decides which menu to show has this comment. It seems like the API is having trouble deciding which one was the more recent if both are created during the same second? What do you think we should do?
There was a problem hiding this comment.
If it's a legit bug of the API then we should open an issue about it? In the meantime, we can use page.waitForTimeout( 1000 ) as a temporary hack, but make sure to add some FIXME comments referencing the issue above it. Thanks for looking into it! ❤️
MaggieCabrera
added
[Type] Automated Testing
MaggieCabrera
requested review from
scruffian,
jeryj,
getdave,
ajlende and
draganescu
jeryj
approved these changes
Mar 8, 2023
getdave
reviewed
Mar 8, 2023
MaggieCabrera
force-pushed
the
e2e-navigation-default-recent
branch
from
0463f38 to
e0e8910
Compare
|
mmmh, it was passing in my local install but it's happening again that the wrong menu is the one being inserted |
|
I wonder if it is OK that we're testing both the back end and the front end in the same test ... |
I've seen it done elsewhere. I'm not sure if it's a good practice in general, but to me, it makes sense that we make sure results are consistent between the two. |
Why do you think it might not be OK? It's the same system so I think it makes sense to write tests ensuring that updates in the backend reflect the changes in the frontend too. As users, we often manually test the same thing by visiting the front page after editing something in the dashboard as well. |
|
Yes it's all true @kevin940726 :) it just makes tests more complex - wouldn't it be easier if we had a markup which we expect to render in a certain way and only test that? Instead we test that the editor works with the blocks as we want and that it saves what we want and that what we want renders as we expect. |
Ideally, yes! But in WordPress there is often a need to test the frontend because the markup doesn't necessarily represent the final result. I think this is a systematic thing that needs to be solved. We could also split it into two different tests and I will be fine with that too :)
I think this is true in unit tests, but necessarily true in e2e tests. Especially when Playwright supports custom error messages for each |
|
Thank you @MaggieCabrera ! ❤️ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This is part of the effort to re work all the navigation block e2e tests and migration to playwright. This is one of the user stories described in #45199
This test checks that when only two navigation menus, inserting a Navigation block will default correctly to using the most recently created one.
Testing Instructions
Run
npm run test:e2e:playwright -- editor/blocks/navigation.spec.js