Add artifacts upload for the performance tests #48243
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
WunderBart
changed the title
|
Size Change: -1.17 kB (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
kevin940726
reviewed
Feb 20, 2023
kevin940726
added
[Type] Automated Testing
|
Flaky tests detected in bbdd33c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4292982546
|
IIUC, it wouldn't work because we create temp folders for each branch that we test against. Those paths are unavailable from the |
WunderBart
added
the
GitHub Actions
dmsnell
reviewed
Feb 20, 2023
dmsnell
reviewed
Feb 20, 2023
| ); | ||
|
|
||
| throw error; | ||
| } |
There was a problem hiding this comment.
why should we only copy these if we fail? would we not want to upload them unconditionally so we can examine them even when the tests pass?
There was a problem hiding this comment.
AFAIK artifacts are not created for successful tests.
There was a problem hiding this comment.
aha, hm. I guess none of this would fail if we run it unconditionally? you're saying that the test runner doesn't create artifacts when the tests all pass?
just asking here because I've found it useful to analyze successful test results (e.g. the performance traces), but if they're not present then I guess there's no point.
the try/catch here is what holds me up. note that we're catching a lot of cases here, so for example, if the tests pass but curateResults fails, we'll still be copying the absence of files.
this is fine as it is, but I think it'd also be fine to leave the try/catch out of this and blanket copy all the artifacts, even if they don't exist. do you think there's a reason not to do that?
There was a problem hiding this comment.
you're saying that the test runner doesn't create artifacts when the tests all pass?
The artifacts I'm trying to handle here are test failure ones, which include a page screenshot and HTML snapshot (at the moment of failure). This is different from the performance results json that we create only if the tests pass.
the try/catch here is what holds me up. note that we're catching a lot of cases here, so for example, if the tests pass but curateResults fails, we'll still be copying the absence of files.
this is fine as it is, but I think it'd also be fine to leave the try/catch out of this and blanket copy all the artifacts, even if they don't exist. do you think there's a reason not to do that?
We need this try/catch block here because otherwise, it would never get to the part where we copy the created artifacts. I agree, though, that it would be better only to wrap the bit where we run the tests (npm run test:performance ...). I'll make an update.
Let's use the same naming as we do in the E2E jobs.
WunderBart
force-pushed
the
add/perf-tests-artifacts-upload
branch
from
39d517f to
fcf67c6
Compare
This way, regardless which comparison part fails, we'll always be able to check for artifacts if they exist.
This reverts commit 4f32c7a.
|
@dmsnell this one's ready for another go! 🙌 |
dmsnell
reviewed
Mar 1, 2023
| script: | | ||
| const commit_details = await github.rest.git.getCommit({owner: context.repo.owner, repo: context.repo.repo, commit_sha: context.sha}); | ||
| return parseInt((new Date( commit_details.data.author.date ).getTime() / 1000).toFixed(0)) | ||
|
|
||
| - name: Publish performance results | ||
| if: github.event_name == 'push' | ||
| env: | ||
| COMMITTED_AT: ${{ steps.commit-timestamp.outputs.result }} |
There was a problem hiding this comment.
I think there's a mismatch between "COMMITTED_AT" and "commit_details.data.author.data" whereby the author can choose a time for the commit but the actual time of commit is different.
we should also be able to skip the network call from JS and use the commit sha directly with git. it looks like we're trying to get the seconds since Jan. 1, 1970?
-name: Get commit timestamp
if: github.event_name == 'push'
id: commit-timestamp
script: |
# Print the commit timestamp in Unix epoch seconds
git show -s '%ct' $GITHUB_SHAIn this case %at shows the "author time" while %ct shows the "commit time." For common work in this repo the difference is largely when did the author last finish writing the code and when was the code merged into the project.
There was a problem hiding this comment.
Nice catch, PR here #48673
dmsnell
approved these changes
Mar 1, 2023
There was a problem hiding this comment.
Thanks @WunderBart - I left a comment about the git timestamp, and I see you are mostly just moving it instead of changing it, so I don't think it should block this PR. Still, looks like we have needless JS and network calls in there and might be reporting a different value than we indicate we are.
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?
Upload artifacts for the performance tests.
Why?
So that it's easier to debug them.
How?
Add the same action as for E2E tests artifacts upload.
Testing Instructions
CI should upload perf test artifacts when it fails. This commit shows that it works as expected.