Merge to master before running tests

[alfsb]

When a PR is branched from a point before current master, the CI tests are run ignoring any intermediary commits. In other words, tests are running considering one version that will never exist in practice.

To avoid this, run an local git merge from the branched checkout. This is a no-op if there is no commits to be merged.

[cmb69]

When a PR is branched from a point before current master, the CI tests are run ignoring any intermediary commits.

Well, isn't that expected behavior? Is there a particular reason to work-around that only for doc-base?

[alfsb]

Well, isn't that expected behavior?

I would argue is not expected behavior that CI tests test a version of code that will never exist.

#205 (comment)

Taking a branch from before the tip of master is no different of taking five branchs of same tip of master, and then merging then in any order. The first merge will not show this problem, but for the other fours are now effectively branched not from from the tip of master, but from a previous version. And CI will ignore any of this, will test the code excluding any changes to master, and then, when merged, the code merged is not the code tested.

Is there a particular reason to work-around that only for doc-base?

The doc-base is checked out from a branch, all other repos are already checked from master.

[cmb69]

The doc-base is checked out from a branch, all other repos are already checked from master.

I was not particularly referring to doc-*, but rather to "all" GH repos using the checkout action (not only those of the php organization). It seems to me that it is relatively rare that issues arise due to not testing against the HEAD (+ the PR commits), and this can be resolved with a rebase.

That said, I'm not against your suggested change.

[alfsb]

I was not particularly referring to doc-*, but rather to "all" GH repos using the checkout action (not only those of the php organization).

That is something I asked on #205 (comment) , and got no answers.

It seems to me that it is relatively rare that issues arise due to not testing against the HEAD (+ the PR commits), and this can be resolved with a rebase.

It's rare, yet it happened, in doc-base no less.

---

Any CI that uses actions/checkout@v4 assumes that there are no concurrent PRs, or, in alternative, that all CI tests only make sense if executed from a rebased state. Yet, GitHub CI tests are not blocked or discarded on non-rebased PRs. It may be worth raising an issue in GitHub Actions Marketplace, asking for an option to something like merge: master, so projects with many concurrent PRs may have a better experience with GH CI.

[alfsb]

Checking out the checkout action PR and issues, there various discussions for and against merge commit checkout, with the implication that is the merge is the default options. But experience with PR 205 showed it was note the case. After this PR is merged, I will try recreate what happened in PR 205.

[alfsb]

I think I confirmed it. Besides the checkout being made from refs/remotes/pull/217/merge, that implies that some merging being done, and docs mentioning merge commits, after others PRs are merged, this PR started failing CI.

Not because of a merge conflict, but because of an Committer identity unknown error in git -C doc-base merge --no-ff --no-commit origin/master. In other words, the merge found differences between the CI checkout and master, and is trying to create a merge commit for it (even in the presence of --no-commit ...). So if some merging is being done, it ignores master history in some (all?) cases.

Next I will try some fixes for Committer identity unknown that does not involve persisted configurations, to see what the merge command is detecting.

[alfsb]

Sorry for the noise. I think this may be a hard problem to solve, as actions/checkout@v4 uses a refspec that generates a detached HEAD state, which makes it harder to coordinate further fetch/checkout/merges of master.

Also, the problem is really about a cached state. Even after changes are made in master, the CI still generates the same hash for the merge commit, indicating that it may be executing tests in a behind version of code. At the same time, any changes to PR flushes/renews the cached result, so very act of developing a fix "fixes" the problem, at least temporarily. I will try to simulate the problem and the fix in a particular repository.

Meanwhile, I changed the PR to a test that will intentionally fail, if the merge commit targets a hash that is not the head of master.

[alfsb]

An old teste generated:
HEAD is now at 478a8242 Merge c496e97c7b89e491b6b5d2179441ff9dba4cf13a into 07b48068fa77eebcb6c1e2788c9a8026fa1bfcae
All tests pass.

Running test same test after merge 218:
HEAD is now at 478a8242 Merge c496e97c7b89e491b6b5d2179441ff9dba4cf13a into 07b48068fa77eebcb6c1e2788c9a8026fa1bfcae
All tests fail.

After a commit make insignificant chantes on PR:
HEAD is now at 53c56510 Merge b7b547b0877092f81a17eb7257cc634fa7ec49b5 into 0ae92dd1987e98a62c7d464bb0659a9e63ffe794
All tests pass.

So, only to repeat. GitHub actions/checkout@v4 ignores changes on main/master for calculating the commit merge target.

And GitHub actions/checkout@v4 recalculate the commit merge target on PR code changes.

This PR, as it is now, only refuses to run CI when the commit merge target is outdated.