Try to fix performance test failure on trunk #51407
Conversation
|
Size Change: 0 B Total Size: 1.4 MB ℹ️ View Unchanged
|
|
Flaky tests detected in b8e5ab7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5254615426
|
t-hamano
changed the title
I actually think that's a problem in itself, maybe we're remounting the components when entering edit mode and that's something I wanted to avoid initially to have a smooth experience (for example embeds/videos don't reload). In other words, why do we have a new DOM node. I think it might a regression in some PR that was unintentional. |
Perhaps the re-rendering of the DOM is not the underlying problem. As shown below, the video plays seamlessly when switching modes (I understand this to mean that the DOM is not being re-rendered). So it is unclear why Node is detached... 3f5dc38713a9413345a0ecc014e8f3f3.mp4 |
|
FWIW this was also happening in a lot of playwright tests before. We fixed it by simply waiting for the iframe to be ready instead of jumping into the rabbit hole. Possibly the fix could serve as an inspiration here? |
While migrating to Playwright, I also noticed that one more click is now needed to get to the target paragraph. That extra layer seems to be the The click-path to the paragraph that worked for me was:
|
|
I think I understand more now. It seems previously it didn't matter if the paragraph block was actually selected or not, we were just adding a "paragraph" block to the template. But since now the editor is by default in "page mode" (no edits/insertion in the template) we can't really insert blocks in the template. So this is probably a side effect of that PR cc @noisysocks The question now, is what approach should we do to fix this, we have two options:
If we don't want to change the purpose of the metric itself, we should probably do the second but the first one is fine. But it means that potentially we can't compare the graph results with previous numbers of the metric. |
|
I may be missing something, but from the beginning when the typing performance test was added, the codebase seems to indicate that typing is intended to take place within the post content in page mode 🤔 |
|
What was happening (at least for some time now) is that when we clicked the paragraph, it was the "post content" block that got selected and not the paragraph meaning that "inserting a paragraph" would insert it after the post content block, not within. |
|
Okay, I think I may have understood. The base branch commit being compared in the performance test was changed at #51381, but it certainly looks like both of these codes just selected the post content block. Before: debd225 gutenberg/packages/e2e-tests/specs/performance/site-editor.test.js Lines 115 to 122 in debd225 After: 843a305 gutenberg/packages/e2e-tests/specs/performance/site-editor.test.js Lines 118 to 126 in 843a305 |
.github/workflows/performance.yml
Outdated
| @@ -57,7 +57,7 @@ jobs: | |||
| ./bin/plugin/cli.js perf "wp/$WP_MAJOR" "$PREVIOUS_RELEASE_BRANCH" "$CURRENT_RELEASE_BRANCH" --tests-branch $GITHUB_SHA --wp-version "$WP_MAJOR" | |||
|
|
|||
| - name: Compare performance with base branch | |||
| if: github.event_name == 'push' | |||
| if: github.event_name == 'pull_request' | |||
There was a problem hiding this comment.
This should be reverted right?
There was a problem hiding this comment.
Yes, this change was made temporarily to allow pull requests to also run the performance test compared with base branch. Unnecessary changes were reverted in 467f513.
There was a problem hiding this comment.
Just noting that changing the event name like that in PRs had some unwanted side effects, basically all the temporary commits in this PR started logging numbers in codevitals.run
I think we should stop doing that in the future. (at least for the part that calls the log script)
There was a problem hiding this comment.
Do we have some sort of permission check as GH Actions does for pull_request and pull_request_target? Does this mean that anyone opening a PR could potentially skew the metrics or even possibly run malicious code in codevitals? Or is it scoped only under maintainers in this repo? Asking to be sure this is not a security risk. Please ignore me if I misunderstood the comment, or if this has already been thought of though!
There was a problem hiding this comment.
Does this mean that anyone opening a PR could potentially skew the metrics or even possibly run malicious code in codevitals?
Anyone that has Gutenberg permissions, If I'm not wrong, fork PRs don't run CI unless approved.
* Try to fix performance test failure * Use more semantic manipulation * Revert unnecessary changes
What?
This PR fixes a problem in trunk where performance tests compared with base branch fail.
Why?
The GitHub Action log shows:
The code indicated by this error is:
gutenberg/packages/e2e-tests/specs/performance/site-editor.test.js
Lines 145 to 156 in 1746e8c
From the Artifacts snapshot, it appears that the site editor is successfully switching to edit mode, but is failing to click on the paragraph block:
I expected that when switching to edit mode, the re-rendering of the DOM would have caused the previously defined
firstParagraphto be detached (one source). I do not know the exact cause of this problem, but one factor might be that the commit in the base branch was changed by #51381.How?
After switching to edit mode, I had puppeteer look for the first paragraph block again. In addition, the if statement is temporarily changed in order to run the performance test compared with base branch in PR as well.
Testing Instructions
In the GitHub Actions performed on this PR, confirm that the performance test compared with base branch is successful.