Performance tests: Refactor use of ENV to simplify test runner logic#45255
Performance tests: Refactor use of ENV to simplify test runner logic#45255
Conversation
|
Size Change: 0 B Total Size: 1.29 MB ℹ️ View Unchanged
|
eaaede8 to
10f0c07
Compare
29b1c97 to
5eba247
Compare
| await git.raw( 'fetch', '--depth=1', 'origin', branch ); | ||
| } | ||
|
|
||
| await git.raw( 'checkout', branches[ 0 ] ); |
There was a problem hiding this comment.
How is the logic here different than the git clone call that was removed?
There was a problem hiding this comment.
one why reason I pulled out the custom git abstraction was because I found it hard to follow what was actually going on by jumping around the various functions and modules, and then I'm not comfortable making changes to the abstraction given my limited understanding of what else relies on it.
the big change here is that all fetches are shallow and that we're not passing --no-single-branch. I'm guessing, because there's no record in the codebase for why --no-single-branch was added, that someone added it when one of the referenced branches didn't appear in the shallow clone. however, that option is going to pull in around 1700 commits right now based on the repository today and that amounts to a jump from roughly 20MB downloaded in the clone for what we need to 190MB for all the other things we don't. that also accounts for several minutes of runtime for the job.
later in #45327 (where I'm collecting the set of successful experiments) I'd like to first fetch from the copy of the repo already cloned in previous step and avoid making network calls during the job run. this work alone accounts for around six minutes of runtime and cuts in half the variability of runtime duration.
because the git abstraction creates directories on clone, because it creates its own git object on every call, and because it doesn't return that git object we can't easily make minor adjustments in how we are using git, which has made these kinds of changes hard to setup or code without pulling in simple-git anyway in addition to the abstraction.
There was a problem hiding this comment.
Should we remove it entirely. I guess it's still used by one other command?
There was a problem hiding this comment.
I would be happy to remove it entirely, as I think that its main contribution is a layer of abstraction that adds indirection while failing to aid in comprehension or efficiency. Generally I'm hesitant to remove things for which I don't fully understand their use or motivation, but I could extend this PR to remove it from the other place also.
There was a problem hiding this comment.
removed in 0445494
it's a bigger change than I prefer, and I plan to add some explanatory comments on things that stood out to me while refactoring (such as why we're using git pull to "rest" a branch)
I will request a more careful review than normal to make sure I didn't change anything in the port; already I caught myself accidentally using trunk instead of npmBranchName in one place.
| // @ts-ignore | ||
| await SimpleGit( performanceTestDirectory ) | ||
| .raw( 'fetch', '--depth=1', 'origin', options.testsBranch ) | ||
| .raw( 'checkout', options.testsBranch ); |
There was a problem hiding this comment.
I guess I have the same question here :). It seems you're moving away from the "git" abstraction we had but I'm not understanding the "why"
There was a problem hiding this comment.
I had a similar question that Dennis answered here.
8bf01c4 to
e6987e7
Compare
|
Can someone familiar with project releases give this a look-over? I'm not entirely sure how to verify that this doesn't accidentally break package and plugin releases. |
| * @TODO: What is our goal here? Could `git reset --hard origin/${pluginReleaseBranch}` work? | ||
| * Why are we manually removing and then adding files back in? |
There was a problem hiding this comment.
What is our goal here? Could
git reset --hard origin/${pluginReleaseBranch}work?
I was puzzled by the same question when I last touched this file. Let's give your suggestion a try 👍
Why are we manually removing and then adding files back in?
I think it's mostly to cover any files that were added and/or removed with respect to the other branch?
There was a problem hiding this comment.
@ockham do you think it would be fair to do this in a follow-up PR or make the change here? I'm nervous about smushing the logic change with the refactor in case it turns out that it was necessary to be the way it is.
There was a problem hiding this comment.
Makes sense. Follow-up is fine 👍
bin/plugin/commands/packages.js
Outdated
| * @TODO: Why do we use fetch/checkout/pull here instead of checkout/reset--hard? | ||
| * Is the fetch necessary at all? Do we expect the remote branch to be | ||
| * different now than when we started running this script? |
There was a problem hiding this comment.
We've had some issues that I believe were due to a scenario like that, see #30615. I'd lean towards keeping the fetch; let's maybe update the comment.
There was a problem hiding this comment.
Ah thank you. That makes sense, especially here where we're back-porting the changes and need to push something fast-forwardable. I'll add a note.
e6987e7 to
ab6fec6
Compare
bin/plugin/commands/performance.js
Outdated
There was a problem hiding this comment.
Could use await git.init().addRemote( 'origin', config.gitRepositoryURL ), but I guess you've covered that below.
There was a problem hiding this comment.
thanks. I tried changing it but there's a thing I don't like about this pattern which is that SimpleGit is making us guess which functions it has chosen to abstract and which not to, and then we need to guess what that abstraction does.
case in point, we have SimpleGit.addRemote() but not SimpleGit.showRemote(). So while .init() makes sense, I feel like the raw version of remote add does far less to obscure what's going on, even if in the end it does the same thing as raw does.
I'd like to leave this as is for now until and unless we see a compelling reason why using this middleman/new term is more useful than using the more true git vocabulary and idioms.
bin/plugin/commands/performance.js
Outdated
There was a problem hiding this comment.
await git.fetch( origin, branch, [ '--depth=100' ] );
55d19c2 to
a6bdaf4
Compare
ockham
left a comment
There was a problem hiding this comment.
I've tested publishing GB on my fork, and it seems to work (and run performance tests). LGTM -- let's maybe leave a note in #core-editor to make sure people are aware of this change in case something goes wrong after all next time a release is published.
5588046 to
a9feb0c
Compare
In this patch we're refactoring how the performance tests are configured, relying on the ENV values set by Github Actions and adding new semantic terms for existing implicit conditions. Additionally, the custom `git` abstraction has been removed and replaced with `SimpleGit` for a more concrete represetnation of what is being performed during the test setup. This change will enable more complicated uses of `git` for optimizing the run script, and by moving out of the abstraction it will minimize any changes based on that kind of `git` usage. There should be no functional changes in this patch and only the code surrounding test setup should change. The term "branch" is replaced in a few places with "ref" since this script is designed to accept any `git` ref pointing to commits, and in fact has been in daily use with a mix of tags, commit SHAs, and branch names.
a9feb0c to
e112e26
Compare
|
@dmsnell, I started seeing the following errors for Playwright and Performance tests actions: Screenshot
|
|
@Mamaduka I think that error is the GitHub problem being talked about here https://wordpress.slack.com/archives/C02QB8GMM/p1668497600444079 |
|
I saw the following issue introduced with this refactor: 
I'll try to resolve it, as it looks like the config entry is read from a wrong place now. |
|
I resolved the previous issue, but now I see the following: 
|
|
I fixed everything, I will open PR tomorrow with improvements to the npm publishing task. |
|
@gziolo please add me if you like on that PR. also if you want any help I'm happy to try and be of service. |
|
The fix is available at #53565. |
What?
Refactors and de-abstracts the performance test runner, relies on ENV values set by GitHub Actions to configure test branches for PR test runs.
Why?
As part of an ongoing effort to optimize the performance tests this change de-abstracts some of the interface to the runner script and refactors to minimize future changes which impact how we checkout/clone the referenced
gitrepository and branches.By rearranging the code here we can more easily take advantage of situational advantages when checking out code, such as reusing the existing local
gitcheckout provided by earlier steps in the GitHub Actions job.There are several new semantic terms introduced to replace implicit logical conditions in hope of making the flow through the test suite clearer; this is important because the test suite currently serves at least three separate purposes: running on PRs to predict impact of new changes; running after PR merge into
trunkto measure historical performance trends; and running against major releases to track impact on performance.How?
gitclient directly instead of using thegitabstraction in the test utils, making it clearer what exactly is happening and making it easier to modify that behavior.There should be no functional or performance changes in this PR. This is a code refactoring in anticipation of further changes which should bring positive impact on test runtime.