[PR flow] vtests do not compare PR changes

[jeetee]

As found in #13992 (comment) and following comment by @cbjeukendrup when we run vtests we compare the PR HEAD (correct) against github.event.pull_request.base.sha (incorrect). The latter is the HEAD of the PR-base-branch (so for us current master).
As a result, when master was updated since PR branch creation a PR might fail on vtests even if it didn't change anything with relation to them.

I've started a discussion in the github community for this. In the meantime it looks like perhaps we could use git merge-base --fork-point <PR-merge-target> to get the correct comparison point to generate the reference images for vtests.

[AntonioBL]

If I understand correctly, in CI runs the PRs are applied as merge commmits to current master branch.
Therefore, if we compare the vtests of this run to the vtests of the master branch, I think we are actually isolating the behavior of the changes in the PR, if there are no problem in the master branch.
Taking the example in the link:
(a) 0 --- 1 --- 2
(b) + 3 --- 4
where (a) is the master branch (0, 1, 2 are commits), and (b) is the branched fork (with commits 3, 4, and based on 0).
So in the CI process, the PR from (b) is first applied to (a), so that we test and have the image results from
0 --- 1 --- 2 --- 3 --- 4
and this is compared against the results of 0 --- 1 --- 2.

Is that correct or am I missing something?

I think the hidden assumption is that master branch should not have problem by itself of vtest generation.

[jeetee]

@AntonioBL It doesn't (refer to the linked discovery thread).

(master) 0 --- 1 (change vtest) --- 2
(PR head) + A ------------------- B --- C

What seems to be compares is PR Head (0 - A - B - C) against master at the moment of PR push trigger (0 -1 -2)
PR did not change vtests, but the merge test failed due to master having changed them.

I see two possible solutions:
(A) as proposed above, run the vtests comparision between PR Head (0 - A - B - C) and PR base (0)
(B) as you thought it worked, run the vtests comparison between master at trigger (0 - 1 - 2) with merge commit (0 - 1 - 2 - A - B - C)

Both scenarios would result in comparing only the changes caused by the PR, with your proposal (B) being even better than mine in that it also tests for knock-on integration caused issues.

[cbjeukendrup]

@jeetee I believe (B) is how it is intended (by GitHub) to work; at least about that merge commit I'm pretty sure. But apparently it does not always work like that. I guess that it doesn't take the merge commit in those cases but the PR head instead. When the PR is in a conflict with the master branch, then that's understandable. But as far as I know, that was not the case with your PR, was it? So either it is a temporary failure by GitHub, or there is some other reason why it didn't take the merge commit, or this whole theory is not true.

[jeetee]

I'm half hoping that it indeed was a hiccup from GH (and half would've expected more bug reports about this if it wasn't).

Given that we can work around it anyway by rebasing, I propose we can consider lowering the priority on this task. After all, this is to my knowledge only the 3rd or 4th time it arose in the 6-ish years I've been contributing to MuseScore.

[jeetee]

I've been notified of https://github.com/orgs/community/discussions/59677#discussioncomment-10782359 which seems to indicate a change by GitHub since this September, which should enable us to perform the option (B) as mentioned above.

[cbjeukendrup]

I had forgotten about the existence of this issue. But recently, I attempted to implement a fix in #24847 / 75f31d2 (not yet merged).
(apparently, judging from the next comment in that discussion, the comment that GitHub would have fixed this was too optimistic…)