-
Notifications
You must be signed in to change notification settings - Fork 2
fix: accept destination dev-branch ancestors in integration history check (PTFE-3343) #271
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3768,6 +3768,88 @@ def test_integration_branch_and_source_branch_force_updated(self): | |
| with self.assertRaises(exns.BranchHistoryMismatch): | ||
| self.handle(pr.id, options=self.bypass_all, backtrace=True) | ||
|
|
||
| def test_history_mismatch_nondiverged_dev_branch(self): | ||
| """A manually-resolved integration branch on a development branch that | ||
| has not diverged from its parent must not trigger a false | ||
| BranchHistoryMismatch. | ||
|
|
||
| development/4.4 is branched off development/4.3 with no commit of its | ||
| own, so both point at the exact same commit ``D``. A PR targeting | ||
| development/4.3 cascades into development/4.4. When the w/4.4 | ||
| integration branch is manually resolved with a forced merge commit (as | ||
| an operator does when the higher development branch must ignore the | ||
| cascaded change, e.g. a `merge -s ours`), that merge commit's first | ||
| parent is ``D`` -- the commit shared with development/4.3. ``D`` is | ||
| part of the destination development branch and must be accepted. | ||
|
|
||
| """ | ||
| # development/4.4 == development/4.3 (no commit of its own) | ||
| self.gitrepo.cmd('git checkout development/4.3') | ||
| self.gitrepo.cmd('git checkout -b development/4.4') | ||
| self.gitrepo.cmd('git push -u origin development/4.4') | ||
|
Comment on lines
+3787
to
+3789
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could pull that into a tiny helper (e.g. branch_nondiverged_dev('4.3', '4.4')) so the two tests only differ on the part that matters (manual merge vs late-PR auto-merge). Not blocking tho |
||
|
|
||
| pr = self.create_pr('bugfix/TEST-00001', 'development/4.3') | ||
| # First evaluation creates and pushes the integration branches. | ||
| with self.assertRaises(exns.BuildNotStarted): | ||
| self.handle(pr.id, | ||
| options=self.bypass_all_but(['bypass_build_status']), | ||
| backtrace=True) | ||
|
|
||
| # Manually resolve w/4.4 with a forced merge commit whose first parent | ||
| # is development/4.4's tip (== development/4.3's tip == D). | ||
| self.gitrepo.cmd('git fetch --all') | ||
| self.gitrepo.cmd('git checkout w/4.4/bugfix/TEST-00001') | ||
| self.gitrepo.cmd('git reset --hard development/4.4') | ||
| self.gitrepo.cmd('git merge --no-ff origin/bugfix/TEST-00001 ' | ||
| '-m "manual resolution of w/4.4"') | ||
| self.gitrepo.cmd('git push -f origin w/4.4/bugfix/TEST-00001') | ||
|
|
||
| # Re-evaluation runs the history check on the manual merge commit. It | ||
| # must not raise a false BranchHistoryMismatch. | ||
| with self.assertRaises(exns.SuccessMessage): | ||
| self.handle(pr.id, options=self.bypass_all, backtrace=True) | ||
|
|
||
| def test_history_mismatch_nondiverged_dev_auto_integration(self): | ||
| """When development/x.y+1 has not diverged from development/x.y (same | ||
| tip D) and the PR's feature branch was branched one commit before D, | ||
| bert-e auto-creates a w/x.y+1 integration branch via a real merge | ||
| commit (feature and D are siblings, not fast-forwardable). The merge | ||
| commit's first parent is D, which the git-log boundary excludes from | ||
| acceptable_parents. On re-evaluation the history check must accept D | ||
| as an ancestor of development/x.y+1 and not raise a false | ||
| BranchHistoryMismatch. | ||
|
|
||
| This exercises the same fix as | ||
| test_history_mismatch_nondiverged_dev_branch but via the auto-created | ||
| path rather than manual operator intervention. | ||
|
|
||
| """ | ||
| # development/4.4 == development/4.3 (no commit of its own) | ||
| self.gitrepo.cmd('git checkout development/4.3') | ||
| self.gitrepo.cmd('git checkout -b development/4.4') | ||
| self.gitrepo.cmd('git push -u origin development/4.4') | ||
|
|
||
| # Feature branch created one commit behind development/4.3's tip so | ||
| # that the merge for w/4.4 cannot fast-forward and produces a real | ||
| # merge commit whose first parent is development/4.4's tip (== | ||
| # development/4.3's tip). | ||
| parent_sha = self.gitrepo.cmd('git rev-parse development/4.3^').strip() | ||
| create_branch(self.gitrepo, 'bugfix/TEST-00001', | ||
| from_branch=parent_sha, file_=True) | ||
| pr = self.create_pr('bugfix/TEST-00001', 'development/4.3', | ||
| reuse_branch=True) | ||
|
|
||
| # First evaluation creates and pushes the integration branches. | ||
| with self.assertRaises(exns.BuildNotStarted): | ||
| self.handle(pr.id, | ||
| options=self.bypass_all_but(['bypass_build_status']), | ||
| backtrace=True) | ||
|
|
||
| # Re-evaluation runs the history check on the auto-created merge | ||
| # commit. It must not raise a false BranchHistoryMismatch. | ||
| with self.assertRaises(exns.SuccessMessage): | ||
| self.handle(pr.id, options=self.bypass_all, backtrace=True) | ||
|
|
||
| def test_success_message_content(self): | ||
| pr = self.create_pr('bugfix/TEST-00001', 'development/5.1') | ||
| try: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,9 +101,18 @@ def update_integration_branches(job, wbranches): | |
| # * the wbranch, | ||
| # * the previous integration branch, | ||
| # * the target development branch. | ||
| # | ||
| # A `<dev>..<branch>` diff excludes the boundary commit, so when the | ||
| # destination development branch has not diverged from its parent (it | ||
| # points at the same commit), the shared tip is in none of the sets | ||
| # above even though it legitimately belongs to the target development | ||
| # branch. Accept any parent that is an ancestor of the destination | ||
| # development branch to cover that case. | ||
| acceptable_parents = prev_set | dst_set | wbranch_set | ||
| for rev in wbranch_set: | ||
| if not all(p in acceptable_parents for p in rev.parents): | ||
| if not all(p in acceptable_parents or | ||
| wbranch.dst_branch.includes_commit(p) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why includes_commit rather than just adding the dev tip to dst_set (the boundary that .. drops)? I think includes_commit is the better choice , just confirming it's deliberate and that wbranch_set stays small enough that a per-parent merge-base is a non-issue. |
||
| for p in rev.parents): | ||
| raise exceptions.BranchHistoryMismatch( | ||
| integration_branch=wbranch, feature_branch=feature_branch, | ||
| development_branch=wbranch.dst_branch, commit=rev, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.