Fix unit test coverage reports, add E2E flake reports#5668
Conversation
|
I see that this fixes Codecov coverage publishing, which is great. We haven't had the I also really like how the E2E tests are summarized now. Color output, and you can expand each test to see its details. I think we should do the same for unit tests, but that will require adding gotestsum to the build submodule. I'm not sure about Buildpulse yet. It seems like maybe it won't work on PRs? It needs a token to upload coverage, but PRs don't get access to the token. I'd like to merge it anyway and try it out. |
01eb15b to
7f63bc9
Compare
This generates JUnit files that are used to detect flaky tests. It uses gotestsum to generate the files. As part of this change I've made gotestsum lower the verbosity of E2E tests - I believe we'll only get a detailed summary of test failures. Signed-off-by: Nic Cope <[email protected]>
Without this token we're being severely rate limited when uploading coverage to Codecov. Signed-off-by: Nic Cope <[email protected]>
Signed-off-by: Nic Cope <[email protected]>
phisco
left a comment
phisco
left a comment
There was a problem hiding this comment.
LGTM in theory, but I couldn't manage to get access to buildpulse, should i be able to see something just by logging in with my gh user?
jbw976
left a comment
jbw976
left a comment
There was a problem hiding this comment.
Very cool @negz, thanks for taking action to start driving our e2e tests towards higher quality! 😎
This looks like a good place to start at least, hopefully buildpulse will work for us since that functionality would be helpful right now. Left one comment about further troubleshooting the error with uploading buildpulse results.
Also, I can access the buildpulse account OK on https://app.buildpulse.io/@crossplane with just my GH account, so perhaps organization level perms are needed @phisco - I don't see any configuration options for that, but the UI is quite sparse so far...
| with: | ||
| account: 45158470 | ||
| repository: 147886080 | ||
| key: ${{ secrets.BUILDPULSE_ACCESS_KEY_ID }} |
There was a problem hiding this comment.
It seems like maybe it won't work on PRs? It needs a token to upload coverage, but PRs don't get access to the token.
Looks like you've configured this correctly given the buildpulse usage docs.
Weird that the error message in Publish e2e test flakes indicates an environment variable is missing:
missing required environment variable: BUILDPULSE_ACCESS_KEY_ID
Is it worth trying to also set this value to an env: key as per github docs, even though buildpulse docs only show it under the with: key? 🤔
There was a problem hiding this comment.
looking at other people's pipelines it looks like it should work. But it's strange that in their case it shows them as set:
While here it doesn't:
There was a problem hiding this comment.
I wonder if it's not some gha shenanigan and it'll work once it's on main 🤔
There was a problem hiding this comment.
aha! Anyone with collaborator access to this repository can use these secrets and variables for actions. They are not passed to workflows that are triggered by a pull request from a fork.
There was a problem hiding this comment.
Unfortunately it looks like it'll only work when they implement tokenless upload as codecov did here. But given that it's actually ignored, as even if it's failing, that step is marked as successful, we can leave it as is, maybe one day it'll be fixed upstream and will work also for PRs, or maybe it's already working fine, but I don't know where to look at 😅
There was a problem hiding this comment.
Thanks for investigating @phisco! so since CI is run on master after each PR is merged, we'll probably start getting data on history master and that will be a good place to start 💪
Similar to what Ginkgo/Gomega had out of the box, just saying 😅 |
|
Sweet, we're starting to get data from master now: https://app.buildpulse.io/@crossplane/crossplane |
|
Also opened a related umbrella issue for e2e reliability: #5671 |
3 participants
Description of your changes
This PR:
As part of setting up Buildpulse I've configured our E2E tests to be less verbose when everything is going well. I believe they'll still give us plenty of context for failed tests.
I have:
make reviewableto ensure this PR is ready for review.Added or updated unit tests.Added or updated e2e tests.Linked a PR or a docs tracking issue to document this change.Addedbackport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.