Fix flaky Nav block user permissions e2e test #38025
Merged
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
getdave
added
[Block] Navigation
|
Size Change: 0 B Total Size: 1.13 MB ℹ️ View Unchanged
|
8 tasks
|
@talldan Could you do me a favour and (assuming you agree with the premise of this PR) restart the tests for runs 4 & 5, checking off s required in the PR desc. If all good then I think we are ok to merge this one. |
kevin940726
reviewed
Jan 18, 2022
Comment on lines
+218
to
+228
| // Creation of the contributor user **MUST** be at the top level describe block | ||
| // otherwise this test will become unstable. This action only happens once | ||
| // so there is no huge performance hit. | ||
| contributorPassword = await createUser( contributorUsername, { | ||
| role: 'contributor', | ||
| } ); |
There was a problem hiding this comment.
Maybe there's a bug in the e2e helpers so that switching to a newly created user is unreliable? Otherwise, I can't think of any other reason why this has to be at the root level.
loginUser doesn't check if the submission is successful, it only tries to fill out the login form and press the login button. Perhaps we can add an assertion in the helper and/or throw a meaningful error message as to why it fails.
The same goes createUser. It doesn't check if the action is successful either. The password it returns is auto-generated before the submission, so checking its existence isn't enough either.
I'm guessing maybe one of these helpers failed at some point and we didn't catch it. The reason it's less unreliable in top-level describe block is still unknown though.
There was a problem hiding this comment.
It'd definitely be good to try to understand the root cause, as otherwise it'll be difficult to use this utility in other tests.
There was a problem hiding this comment.
Probably a good opportunity to rewrite these utils with rest or something similar too 🙈 .
There was a problem hiding this comment.
Oh, I didn't realise it wasn't already using REST. Yeah, I definitely think that'd be a good idea.
There was a problem hiding this comment.
Sounds good. Are we able to ship this fix now though so we have test coverage and then circle back to making it REST?
There was a problem hiding this comment.
Yes, absolutely. I have a draft PR opened at #38060 which might be helpful for that matter and I'll work on it next.
talldan
reviewed
Jan 18, 2022
| @@ -211,6 +211,18 @@ async function getNavigationMenuRawContent() { | |||
| // Disable reason - these tests are to be re-written. | |||
| // eslint-disable-next-line jest/no-disabled-tests | |||
| describe( 'Navigation', () => { | |||
| const contributorUsername = 'contributoruser'; | |||
There was a problem hiding this comment.
I wonder if we should make the username unique by adding some randomness, as one of the reasons WordPress would prevent creation of a user is a duplicate username, and that might happen if clean up of this test fails, or possibly another test using the same username.
There was a problem hiding this comment.
I added randomness using lodash. Does that work?
There was a problem hiding this comment.
This might be slightly over-engineered. I'd imagine a better solution would be having a proper error message if that happens. We are already running our tests in-band, so such things shouldn't happen. If they happen, it might be because our test cases aren't being correctly cleaned up, and we should fix them. But it's not that it's an anti-pattern or anything like that, I don't mind landing this 😅 .
There was a problem hiding this comment.
True, it shouldn't happen, but if it did I feel like it would still be really obscure and hard to track down. Better error messages would be good though.
I don't think _.uniqueId would do the job anyway, as that's just an incrementing integer. Another test in a new process would generate the same integer.
|
I notice this is failing quite a lot, so I've skipped the test for now until this is ready to merge - #38099 I'm happy to go ahead with this change as a temporary step to stabilise the test, but that first comment from @kevin940726 about the |
Acts a safeguard against duplicate users.
getdave
force-pushed
the
fix/nav-block-low-permission-users-flaky-e2e-test
branch
from
afeb5fd to
a60a47e
Compare
Nice one. Thanks.
I think @kevin940726 might be mistaken. It's already in the Also I rebased the branch and also added randomness to the username. Let me know if this can merge or if there's more that needs to happen. |
kevin940726
approved these changes
Jan 20, 2022
|
Just re-running this a few times to ensure it's still stable. @talldan when you come online if this is still ✅ feel free to merge. |
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.
Description
The user permissions test for the Nav block have been flaky recently. I wondered why this was because when I committed they weren't - I ran several test runs on CI to confirm that fact.
After some digging I noticed that a92586c moved the creation of the contributor user to be local to the
describeblock where the permissions tests are performed. However, when @ockham and I originally debugged this test (when it was created) we ascertained that the contributor user creation had to happen at the top/root leveldescribeblock otherwise the test was unstable. We don't know why that is but it is probably a quirk of Puppetter.This PR moves the user creation back to the root level
describein the hope that will make the test stable again.Closes #37604
How has this been tested?
Run the CI e2e tests and make sure that the x2 tests below do not fail:
shows a warning if user does not have permission to edit or update navigation menusshows a warning if user does not have permission to create navigation menusWe should run the CI on this PR 5 times to be reasonably sure this fix does actually produce a more stable test. Please check off below when you see a valid test run:
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).