ibc-go
ibc-go copied to clipboard
cosmos
Make e2e tests a required check
Summary
Currently, e2e tests aren't a required check before merging/automerging. This could cause some headaches in cases where we merge from main and then automerge, an error might be introduced which isn't noticed until after the automerge has been performed.
This wasn't done previously due to some non-deterministic failures which, as far as I'm aware, have been resolved.
Proposal
Make e2e tests a required check before merging.
For Admin Use
- [x] Not duplicate issue
- [x] Appropriate labels applied
- [ ] Appropriate contributors tagged/assigned
Looks like we need to explicitly skip fork and non-fork e2e's in certain situations. Right now, enabling e2e's for all will cause pr's to block as fork e2e's don't run on a non-fork. See github docs. I guess we basically have to create a no-op workflow for each of the cases. So fork pr's need a no-op e2e-tests workflow and non-fork pr's need a e2e no-op workflow for each test run
Maybe we can start by picking just a handful of required e2e's (like transfer), this should stop broken e2e's from merging
hopefully this should be straight-forward enough that I can add these workflows until Monday.
Some additional comments here.
It seems that currently, due to the way e2e's are set up, we aren't able to mark them as required. The problem is due to the fact that e2e's for normal PRs and e2e's for forks are launched differently:
- e2es for normal PRs basically launch sub-jobs that get executed. This means that from Github's POV, the job that launched them is independent from the launched jobs. Skipping the main job does not skip the sub jobs but instead marks them as pending.
- e2es for fork-prs are run as part of the
e2ejob using a matrix, reference. The end result being that when thee2ejob gets skipped, they all get skipped and are marked as successful.
As a result, we can't set the e2e job as a requirement for branch protection, using it results in false-positives since, for example, on a normal PR we'd have:
- e2e job marked as required. 2, e2e job for the fork-pr workflow is skipped and considered passing.
- e2e job marked as required is then considered successful even if a particular e2e test fails.
Renaming these jobs would be part of the solution but still wouldn't address the issue for non-fork PRs since having e2e being required for them doesn't mean that all sub-jobs should be required.
The best solution here, IMO, is two step:
- add an additional input to the
e2e-test-workflow-call.ymljob that can tell it to skip itself if the job that launches it is to be skipped. This way we can mark each individual e2e test as required and have them be skipped in the case where they should not be ran. - change the requirements for branch protection. For fork-e2es we can use the new name for the job (
e2e-forkfor example) and that would cover all individual e2e tests. For non-fork e2es we'd need to add each individual job as required in the branch protection rules.
Thoughts @colin-axner, @chatton?
I further looked at this during the weekend and got a hang of things after creating a separate repo just to figure things out (ref: some findings in the repo md).
I've opened #3884 to get things working the way we want to. To boil things down, the issue is that for certain jobs (matrix jobs, jobs that use uses, probably others), what github sees as the job being ran differs based on if the job is skipped or if it actually executes. I'm hopeful the example in the linked .md covers the problem nicely.
I got things working the way we want to in that linked PR by requiring both e2e-tests-succeed and e2e-fork-succeeds in the branch protection rules. Jobs are skipped/marked as passing/failing as required, nothing stays in a pending state.
The other issue left is marking these new jobs as being skipped when PRs that target docs are opened, #3818 can cover that after #3884 is merged.
This issue was mistakenly reopened during some integration work, so closing again now.
