Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clone the PR's merge branch vs head in e2e tests #483

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bbrowning
Copy link
Contributor

This configures actions/checkout to clone the PR's merge branch (instead of the PR's head), to align with how unit/functional tests work and ensure that all of our tests are testing against the same codebase - the state of the code as it would look after the PR merges, to get an accurate picture of whether things will pass if this merges or not.

Related to #482

@mergify mergify bot added the CI/CD Affects CI/CD configuration label Jan 16, 2025
@bbrowning
Copy link
Contributor Author

So, this might fix 482 but it's one of those things we won't know until we merge it and see what happens. I tested this manually in a separate PR and it fixed things, but testing these changes manually involves changing the trigger conditions for the workflows and doing that can influence the exact ref checked out by actions/checkout. This definitely fixes the issue for anything use pull_request dispatching, but for pull_request_target like we use I'm not 100% sure if that will also use the PR's merge branch or instead only use the base branch when cloning the repo in actions/checkout.

@bbrowning
Copy link
Contributor Author

After spelunking in the GitHub actions/checkout repo, it appears that when using pull_request_target then actions/checkout will by default checkout the base branch and NOT the pr merge commit. The way I was manually testing this was not using pull_request_target because that can only be tested once changes have merged. So, I'm going to change the code to fix this a different way, by going back to the explicit checkout we had but pointing it to the PR merge. That should allow me to test this in a pull_request dispatch somewhere and have confidence that it will work in a pull_request_target dispatch.

@bbrowning bbrowning marked this pull request as draft January 17, 2025 00:28
This configures actions/checkout to clone the PR's merge
branch (instead of the PR's head), to align with how unit/functional
tests work and ensure that all of our tests are testing against the
same codebase - the state of the code as it would look after the PR
merges, to get an accurate picture of whether things will pass if this
merges or not.

Fixes instructlab#482

Signed-off-by: Ben Browning <[email protected]>
@bbrowning bbrowning force-pushed the e2e-pr-merge-branch branch from d17bb74 to b1979c6 Compare January 17, 2025 15:37
@bbrowning bbrowning marked this pull request as ready for review January 17, 2025 15:40
@bbrowning
Copy link
Contributor Author

Ok, this is ready for review. The tldr of this change is that instead of checking out pull/123/head we now checkout pull/123/merge which is a synthetic merge commit created by GitHub server-side that represents the state of the code if the PR was merged into the target branch. This matches the code our unit tests run against, and prevents weird situations I ran into where unit tests pass but e2e tests fail in an unexpected way because one is testing against the state the repo would be in after the PR merges (PR's merge branch) and one is testing against the state of the repo when the PR was opened (PR's head branch). The linked issue #482 has a bit more details for the underlying issue itself.

Note that the default behavior of actions/checkout is to checkout the merge branch for pull_request events, but for pull_request_target events its default behavior is to checkout the base branch (with no code from the PR at all). You'll see I removed the repository reference from our actions/checkout calls when referring to the same repository so that its default behavior applies and we don't get any unexpected surprises in what we checkout if we end up swapping our event from pull_request_target to pull_request at some point.

@bbrowning bbrowning added this to the 0.8.0 milestone Jan 23, 2025
@bbrowning bbrowning added the bug Something isn't working label Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CI/CD Affects CI/CD configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant