Fix Site Editor perf tests #48240
Merged
Fix Site Editor perf tests #48240
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
|
See #48208 (comment), 9bc7099 is also required to fix another failure (there are two issues). Since it was introduced in #48063, @youknowriad might have an idea why it solves the issue and what the solution should be. |
kevin940726
reviewed
Feb 20, 2023
|
Size Change: 0 B Total Size: 1.33 MB ℹ️ View Unchanged
|
|
To genuinely solve this problem, the following additional changes would need to be made. https://github.com/WordPress/gutenberg/pull/48208/files#diff-686c21b911380ccd4d7165b856981854a1dca8bf2084e4e4a5e487219e6afc63R179-R182 @kevin940726 |
|
Missing bits added as per #48240 (comment) and #48240 (review) 🤞 |
youknowriad
reviewed
Feb 20, 2023
| @@ -90,6 +90,7 @@ describe( 'Site Editor Performance', () => { | |||
| await visitSiteEditor( { | |||
| postId: id, | |||
| postType: 'page', | |||
| path: '/navigation/single', | |||
There was a problem hiding this comment.
This is not needed I think.
There was a problem hiding this comment.
Somehow, this was necessary for the E2E test. See this comment and this comment.
There was a problem hiding this comment.
I've reverted it in 87b1109 since the specs failed without it.
There was a problem hiding this comment.
Any idea why it would fail in CI but not locally?
There was a problem hiding this comment.
Maybe you started the PR before my refactoring PR was merged. What if you rebase?
There was a problem hiding this comment.
Good find.
So we do have this code in the Gutenberg plugin that forces the site editor to redirect to "postId:none". But the reasoning for that code is that it's a filter for wp_redirect meaning it only runs if we're redirecting to the site editor. Something we do in WP core (in 6.1 and before) but that redirect is not supposed to happen if we provide a postId and postType in the URL https://github.com/WordPress/wordpress-develop/blob/73f7aa4a149890f2e565ac74d9d49b5258b8b984/src/wp-admin/site-editor.php#L36-L46
So the question here is why the redirect filter is triggering while we do have postId and postType in the URL.
There was a problem hiding this comment.
Tests passed! 🥳 I need to head out now, so feel free to ship.
There was a problem hiding this comment.
Okay, I found out why this is happening on CI. I think it's because it doesn't have some required cookies, yet. You can reproduce it locally in incognito mode. Hope that it will spark some ideas for the solution.
Meanwhile, I think we should just merge this to unblock other PRs. We can figure out why and the proper solution in another PR. WDYT?
There was a problem hiding this comment.
How about trying to remove the code in this comment before merging?
There was a problem hiding this comment.
I found a thing of interest. In Firefox, accessing the following URL will cause a redirect and the page will not be displayed correctly.
Sorry, this was my mistake. I was simply not logged into WordPress 😅 Therefore, this problem also occurs in Chrome. This issue may need to be looked into separately, as this URL should originally redirect to the login page.
youknowriad
reviewed
Feb 20, 2023
|
Flaky tests detected in adbf938. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4228613165
|
kevin940726
reviewed
Feb 20, 2023
ellatrix
reviewed
Feb 20, 2023
| @@ -147,7 +148,7 @@ describe( 'Site Editor Performance', () => { | |||
| '[data-type="core/post-content"] [data-type="core/paragraph"]' | |||
| ); | |||
| await enterEditMode(); | |||
| await canvas().click( | |||
| await canvas().focus( | |||
There was a problem hiding this comment.
Since the path arg is the problem, is this change still needed?
There was a problem hiding this comment.
Yes, it's needed. It's mentioned above in #48240 (comment), in the PR description, and in the original exploration PR, that there are two separate issues in the perf tests. The path one is always failing, while this is a really flaky one. I think we need both to unblock others.
ellatrix
approved these changes
Feb 20, 2023
dmsnell
approved these changes
Feb 20, 2023
There was a problem hiding this comment.
I've added a comment pointing to the discussion here concerning /navigation/single.
For now if it's clears the tests then I encourage a merge to stop blocking unrelated work. In #48262 I've made a crude attempt to revert the new-page-per-e2e-test and see if that's related to these failures (may not have properly applied the revert), though the merge of that PR was ten days ago and the failures seemed to have started closer to four days ago.
ellatrix
force-pushed
the
fix/site-editor-perf-tests
branch
from
2852aac to
e09e90d
Compare
As per #48240 (review) and #48240 (comment)
This reverts commit 5fad8cd10c0765b8054ffdae7f6bd1840c7bba6c.
This reverts commit 87b1109c0517b27ffe8f5bd5fb417a37e7a822d4.
This reverts commit 2b931a8bccfbafa9665e638045c6d1847955d98e.
ellatrix
force-pushed
the
fix/site-editor-perf-tests
branch
from
e09e90d to
ae4b606
Compare
|
I have re-applied the changes mentioned in this comment. Let't try to see if the test passes several times. |
|
The performance test was successful three times. Let's merge! |
t-hamano
added
[Feature] Site Editor
ntsekouras
pushed a commit
that referenced
this pull request
Feb 23, 2023
* Focus on the paragraph instead clicking it See #48208 (comment) * Add missing fixes As per #48240 (review) and #48240 (comment) * Try without the extra path param As per #48240 (review) * Remove obsolete await As per #48240 (comment) * Revert "Try without the extra path param" This reverts commit 5fad8cd10c0765b8054ffdae7f6bd1840c7bba6c. * Revert "Revert "Try without the extra path param"" This reverts commit 87b1109c0517b27ffe8f5bd5fb417a37e7a822d4. * Revert "Revert "Revert "Try without the extra path param""" This reverts commit 2b931a8bccfbafa9665e638045c6d1847955d98e. * Remove unneeded changes * Describe discrepancy of inexplicably-required line in code via explanatory comment. * Use `canvas.focus()` instead of `canvas.click()` --------- Co-authored-by: Kai Hao <[email protected]> Co-authored-by: Ella van Durpe <[email protected]> Co-authored-by: Dennis Snell <[email protected]> Co-authored-by: Tetsuaki Hamano <[email protected]>
|
I just cherry-picked this PR to the wp/6.2 branch. |
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?
Isolating the solution from #48208. See #48208 (comment).
Why?
Site Editor performance tests are currently very flaky.
How?
Focus on the paragraph instead of clicking it.