Size Change: +36 B (0%)

Total Size: 1.71 MB

_{compressed-size-action}

Flaky tests detected in 2eb27e9.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7143561472
📝 Reported issues:

• [Flaky Test] should display the "Manage Fonts" icon #56776 in /test/e2e/specs/site-editor/font-library.spec.js

I think 30% is too much, maybe we should try initially with 20% or even 10%, check the PRs that get failures, and if there's too many, we bump it a bit.

I think 30% is too much, maybe we should try initially with 20% or even 10%, check the PRs that get failures, and if there's too many, we bump it a bit.

@ellatrix, I did test with a 10% threshold and got some failures:

Performance regression detected:
- In post-editor.performance-results.json: minType increased by 10.84% (from 19.1 to 21.17)
- In post-editor.performance-results.json: typeContainer increased by 12.91% (from 8.21 to 9.27)
- In post-editor.performance-results.json: maxTypeContainer increased by 23.93% (from 10.78 to 13.36)
- In post-editor.performance-results.json: minFocus increased by 15.79% (from 37.74 to 43.7)
- In site-editor.performance-results.json: maxType increased by 19.58% (from 41.31 to 49.4)

It made me realize that we probably need some type of non-linear threshold, because the higher the ms value, the more significant the 10% become. For example, 10% for the typeContainer's 9ms average value is well within the drift boundaries, but for the firstBlock's 5 seconds it becomes a significant regression.

Some notes for now since I need to focus on something else for a bit:

• For the non-linear threshold, should is be absolute (ms) or relative (%) values? How do we define them? (e.g. should we use historical data?)
• Should we include the min and max metric values into calculating the threshold?
• We might want to skip min* and max* metrics from the regression check.
• [going to add stuff here if anything comes to mind]

Do you have any thoughts on the above? cc @youknowriad @dmsnell

I'm not sure we should "fail" the tests but I think we should show the changes in a PR comment (similar to the bundle size changes). Can we try that first before failing the tests.

I'm not sure we should "fail" the tests but I think we should show the changes in a PR comment (similar to the bundle size changes). Can we try that first before failing the tests.

I agree this would be a good way of evaulating the check and its parameters. At the same time, we could still fail the PR if the regression is critical, e.g. 50%.

Another idea is to add a column to the comparison results with the relative difference. For example:

Yeah tbh I agree with logging the results in a comment. If it fails people will just rerun the test until it succeeds, which would work if it's somewhere on the border

Thanks @WunderBart

I'm concerned about this change for a few reasons: one is the missing description and motivation for the new gate-keeper. Two is that I don't think we have a good feel yet for what should be rejected and what shouldn't, what is a regression and what isn't. As we've all discussed, our performance tests aren't exactly the most reliable or representative at the moment.

I'm all for adding a new comment though to PRs and updating that comment as the PR's merge-branch is updated. It would help expose our performance metrics to folks and raise awareness of the different measurements we're making.

Ultimately some changes are going to slow things down as a direct result of added functionality: are we to reject that work in blank-and-white terms because it pushed us over some threshold? What if we have some major change that reduces a metric and then it turns out that our speedup broke some part of the app - should we reject our revert PR because it undoes or partially undoes the changes we made?

Personally the unforgiving and often-changing code-linting rejections have been a frustrating experience for me, and I'm worried about adding even more process to dishearten developers who are trying to add valuable contributions to Gutenberg. That is, I love reporting on how this impacts our recorded metrics, I'm skeptical that rejecting PRs is going to provide a net boost to the project. If nothing else, having the comments readily available in the PR might help a reviewer point out performance regressions and leave that call subjective (where I believe it belongs).

Note too that we can hone in on more meaningful rows in the table. @jsnajdr did a good job with is-calypso-fast-yet. It gives a review at a glance of major changes in percentages or absolute metrics, but then we could expand a <details> element to see the full performance changes.

I agree we should start with posting comments on PRs. That's a huge improvement over the current situation where the perf test results are buried deep in the GitHub action's logs.

For meaningful comments, we should also stop reporting numbers like "first paint: 1193.35 ms". Because that precise number is not what we measured. We did X measurements (where X is between 5 and 15?) which have some variance. There are standard methods for reporting the variance -- calculating quartils, detecting outliers and plotting them like:

You need that info to determine whether the differences are real or just noise. For our purposes we don't even need any sophisticated significance calculations, just look at the yellow boxes if they overlap or not.

That's what the comments should report in some simplified form, and also the codevitals chart should eventually show ranges instead of points.

Failing the PR? I think we should never do that. Improve the robots to provide good information, but making decisions should be reserved to humans. If there is some big regression, maybe it was caused by adding some new very valuable feature and the humans decided that the regression is worth it? The robots cannot know.

Failing the PR? I think we should never do that. Improve the robots to provide good information, but making decisions should be reserved to humans. If there is some big regression, maybe it was caused by adding some new very valuable feature and the humans decided that the regression is worth it? The robots cannot know.

Love it.

Looks like we have a consensus on making this about adding a comment with perf changes instead of failing the PR. I'll start working on that and change this PR's title so it's no longer confusing.

Thank you all for all the feedback so far.