I'm reverting for now, as it also seemingly is breaking gerrit CI too:

https://integration.wikimedia.org/ci/job/wmf-quibble-core-vendor-mysql-php81/10068/console

00:03:52.493 There was 1 failure:
00:03:52.493 
00:03:52.493 1) Wikibase\Repo\Tests\WikibaseRepoSchemaTest::testSchemaChangesHaveAutoGeneratedFiles with data set "patch-wb_changes-change_timestamp.json" ('/workspace/src/extensions/Wik...p.json')
00:03:52.493 SQL in /workspace/src/extensions/Wikibase/repo/sql/postgres/archives/patch-wb_changes-change_timestamp.sql for postgres does not appear to be autogenerated. Re-generate it using generateSchemaChangeSql.php.
00:03:52.493 Failed asserting that two strings are identical.
00:03:52.493 --- Expected
00:03:52.493 +++ Actual
00:03:52.493 @@ @@
00:03:52.493  -- Do not modify this file directly.
00:03:52.493  -- See https://www.mediawiki.org/wiki/Manual:Schema_changes
00:03:52.493  ALTER TABLE
00:03:52.493 -  wb_changes ALTER change_time TYPE TIMESTAMPTZ;
00:03:52.493 -
00:03:52.493 -ALTER TABLE
00:03:52.493    wb_changes ALTER change_time TYPE TIMESTAMPTZ;'
00:03:52.493 
00:03:52.493 /workspace/src/tests/phpunit/structure/AbstractSchemaTestBase.php:138
00:03:52.493 /workspace/src/tests/phpunit/structure/AbstractSchemaTestBase.php:120

The revert has merged, so this is presumably resolved now?

Change #1174770 had a related patch set uploaded (by Zabe; author: Zabe):

[mediawiki/extensions/Wikibase@master] Reenable doctrine tests for doctrine update (2nd try)

https://gerrit.wikimedia.org/r/1174770

In T400918#11051058, @Jdforrester-WMF wrote:

I'm reverting for now, as it also seemingly is breaking gerrit CI too:

https://integration.wikimedia.org/ci/job/wmf-quibble-core-vendor-mysql-php81/10068/console

00:03:52.493 There was 1 failure:
00:03:52.493 
00:03:52.493 1) Wikibase\Repo\Tests\WikibaseRepoSchemaTest::testSchemaChangesHaveAutoGeneratedFiles with data set "patch-wb_changes-change_timestamp.json" ('/workspace/src/extensions/Wik...p.json')
00:03:52.493 SQL in /workspace/src/extensions/Wikibase/repo/sql/postgres/archives/patch-wb_changes-change_timestamp.sql for postgres does not appear to be autogenerated. Re-generate it using generateSchemaChangeSql.php.
00:03:52.493 Failed asserting that two strings are identical.
00:03:52.493 --- Expected
00:03:52.493 +++ Actual
00:03:52.493 @@ @@
00:03:52.493  -- Do not modify this file directly.
00:03:52.493  -- See https://www.mediawiki.org/wiki/Manual:Schema_changes
00:03:52.493  ALTER TABLE
00:03:52.493 -  wb_changes ALTER change_time TYPE TIMESTAMPTZ;
00:03:52.493 -
00:03:52.493 -ALTER TABLE
00:03:52.493    wb_changes ALTER change_time TYPE TIMESTAMPTZ;'
00:03:52.493 
00:03:52.493 /workspace/src/tests/phpunit/structure/AbstractSchemaTestBase.php:138
00:03:52.493 /workspace/src/tests/phpunit/structure/AbstractSchemaTestBase.php:120

I am a bit confused why the primary CI passed in the first place.

But I think I know what happened. In the core patch close towards its merge I decided to upgrade doctrine/dbal only to 3.10.0 instead of 4.2.5 since the 4.x causes issues with postgresql autoincrements.

After that I went ahead and re-regenerated the sql files in my patch (which you can see here: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/1174508/4..6).

I did it by ls extensions/Wikibase/repo/sql/abstractSchemaChanges/ | xargs -I{} bash -c "echo {}; php maintenance/generateSchemaChangeSql.php --type all --json extensions/Wikibase/repo/sql/abstractSchemaChanges/{} --sql extensions/Wikibase/repo/sql/".

The problem was that the script generated the sql files to /repo/sql/postgres/patch-wb_changes-change_timestamp.sql instead of /repo/sql/postgres/archives/patch-wb_changes-change_timestamp.sql and I apparently missed moving it to the correct spot and the correct sql is now only untracked on my local repo.

Doing the same again without this mistake should work.

In T400918#11051335, @Zabe wrote:

I am a bit confused why the primary CI passed in the first place.

Yes, me too!

Doing the same again without this mistake should work.

Happy to try that, but ideally I'd want to know how it merged in a bad state.

Change #1175050 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] Reapply "Reenable doctrine tests for doctrine update"

https://gerrit.wikimedia.org/r/1175050

According to phpunit_group_6_databaseless.result.cache from one of the gate-and-submit builds, all the WikibaseRepoSchemaTest tests were still skipped (value 1, aka BaseTestRunner::STATUS_SKIPPED in defects; compare with value 3, aka STATUS_FAILURE, in this result cache from the “reapply” change above) for some reason.

According to that build’s full console, it cloned Wikibase at the mysterious commit b67e7a07a97f927b3ea410f03322773d81514a8f.

INFO:zuul.Cloner.mediawiki/extensions/Wikibase:Prepared mediawiki/extensions/Wikibase repo with commit b67e7a07a97f927b3ea410f03322773d81514a8f

What is that commit? It’s not known to Gerrit or Gitiles and also doesn’t exist in my local clone. My best guess is that it’s some sort of temporary rebase commit due to T390719: Change Gerrit default submit strategy to 'Rebase if Necessary' and allowing content merge; but I don’t see how we can verify that, so for all I know, the gate-and-submit build might not have been testing the Gerrit change at all, but rather some previous tree of Wikibase.git (which might at least explain why the tests were skipped).

We have one more clue in that log:

INFO:quibble.commands:Found repo /workspace/src/extensions/Wikibase with tree a36c019fff2e2f5c2cdabc3cd39a3e142c22df12

Unfortunately, I can’t find that tree in my local Git clone either. In theory, trees should be more “reproducible” than commits, so I tried cherry-picking 153388c6cd (“Reenable doctrine tests…”) onto various earlier Wikibase.git commits, but none of them produced that tree hash. So I still don’t know what tree this build was running on, and whether it actually included the change ostensibly being tested or not.

(For some other repositories, the trees can be found; for instance, the VisualEditor tree 33d191226c836242e177009f0fdbdaee530b273b = 9a60eead05^{tree}, and the MediaWiki core tree 82fc2a6ba3b9c25ec6e6c0aed059cb7fe9617f1c = f11b59de58^{tree} – use git rev-parse f11b59de58^{tree} to show the tree hash on the CLI. So I don’t think the tree hashes are expected to be meaningless / untraceable.)

I tried a few more local cherry-picks after noticing that the gate-and-submit build also included change 1172315 PS 19 (also in Wikibase.git), but still had no success reproducing that tree hash. I think I’m out of ideas at the moment and hope somebody else has more…

In T400918#11053229, @Lucas_Werkmeister_WMDE wrote:

According to that build’s full console, it cloned Wikibase at the mysterious commit b67e7a07a97f927b3ea410f03322773d81514a8f.

INFO:zuul.Cloner.mediawiki/extensions/Wikibase:Prepared mediawiki/extensions/Wikibase repo with commit b67e7a07a97f927b3ea410f03322773d81514a8f

What is that commit? It’s not known to Gerrit or Gitiles and also doesn’t exist in my local clone. My best guess is that it’s some sort of temporary rebase commit due to T390719: Change Gerrit default submit strategy to 'Rebase if Necessary' and allowing content merge; but I don’t see how we can verify that, so for all I know, the gate-and-submit build might not have been testing the Gerrit change at all, but rather some previous tree of Wikibase.git (which might at least explain why the tests were skipped).

That is the merge commit from zuul. It can be found in the parameters - https://integration.wikimedia.org/ci/job/quibble-vendor-mysql-php81/22822/parameters/
Zuul always rebase/merge with master before running tests. T390719 is to rebase on submit, not tests.

Alright, with that and some of the information in mw:Continuous integration/Zuul § Zuul git repositories I was able to get access to the commit (using integration-agent-docker-1040 as an arbitrary host that has the necessary access to contint1002, sorry about that – I’ll try to remember to clean it up later).

lucaswerkmeister-wmde@integration-agent-docker-1040:~$ git clone --depth=1 git://contint1002.wikimedia.org/mediawiki/extensions/Wikibase
Cloning into 'Wikibase'...
[snip]
lucaswerkmeister-wmde@integration-agent-docker-1040:~$ cd Wikibase
lucaswerkmeister-wmde@integration-agent-docker-1040:~/Wikibase$ git fetch origin refs/zuul/master/Z563389a7de154c169b715074eca2ace1
remote: Enumerating objects: 437550, done.
remote: Counting objects: 100% (437550/437550), done.
remote: Compressing objects: 100% (79797/79797), done.
remote: Total 437550 (delta 351029), reused 426613 (delta 341416), pack-reused 0
Receiving objects: 100% (437550/437550), 80.58 MiB | 33.41 MiB/s, done.
Resolving deltas: 100% (351029/351029), done.
From git://contint1002.wikimedia.org/mediawiki/extensions/Wikibase
 * branch                  refs/zuul/master/Z563389a7de154c169b715074eca2ace1 -> FETCH_HEAD
lucaswerkmeister-wmde@integration-agent-docker-1040:~/Wikibase$ git switch -c T400918 FETCH_HEAD
[snip]
lucaswerkmeister-wmde@integration-agent-docker-1040:~/Wikibase$ git rev-parse @^{tree}
a36c019fff2e2f5c2cdabc3cd39a3e142c22df12

So there’s our mystery tree at last. And lo and behold:

lucaswerkmeister-wmde@integration-agent-docker-1040:~/Wikibase$ grep -B1 -A2 setUp repo/tests/phpunit/includes/WikibaseRepoSchemaTest.php
class WikibaseRepoSchemaTest extends AbstractSchemaTestBase {
	protected function setUp(): void {
		parent::setUp();
		$this->markTestSkipped( 'Temporary disabled for gerrit 1174110' );
	}

Somehow, we’ve ended up with a tree where the test is still skipped. The Git log looks like this:

lucaswerkmeister-wmde@integration-agent-docker-1040:~/Wikibase$ git log --oneline --graph | head -15
*   b67e7a07a9 Merge commit 'refs/changes/08/1174508/5' of ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase into HEAD
|\  
| * 145631afaf Reenable doctrine tests for doctrine update
| * bd29e25951 Temporary disable doctrine tests for doctrine update
* | eb4a675dc6 REST: Introduce `SiteLinkGlobalIdentifiersProviderSiteIdsRetriever` service
* | 098d2f5b9e GQL: Support labels of item values
* | 9f0f4590c6 Add ADR 31 on HTTP specifics in use cases
* | a9efdada36 GQL: Support item values
* | a059d5cdea GQL: Allow filtering by statement property
* | 0c4218b207 Localisation updates from https://translatewiki.net.
* | f789f17769 Temporary disable doctrine tests for doctrine update
* | 217b36b1b3 Align uses of snak / data value HTML
* | 369430e28d GQL: Support novalue and somevalue statements
|/  
* 88005fdfad Localisation updates from https://translatewiki.net.

So the “Reenable doctrine tests” commit is included, and it does include a change to WikibaseRepoSchemaTest to remove the skip call – and yet Git does not include it in a list of commits touching that file:

lucaswerkmeister-wmde@integration-agent-docker-1040:~/Wikibase$ git show 145631afaf repo/tests/phpunit/includes/WikibaseRepoSchemaTest.php
commit 145631afafa22050a021c9136b4d56eb56e8c074
Author: Alexander Vorwerk <zabe@avorwerk.net>
Date:   Wed Jul 30 18:47:29 2025 +0200

    Reenable doctrine tests for doctrine update
    
    We regenerate all sql files for this.
    
    This reverts commit bd29e25951bc303895646c08f850ffe812b160bf.
    
    Depends-On: I2bf1093d153bc5ed916045cc73a17df8d3a24150
    Change-Id: I1255dd913eed2c8273f123c4acce947b6d3c03be

diff --git a/repo/tests/phpunit/includes/WikibaseRepoSchemaTest.php b/repo/tests/phpunit/includes/WikibaseRepoSchemaTest.php
index 3c060a24ce..6f740e33bb 100644
--- a/repo/tests/phpunit/includes/WikibaseRepoSchemaTest.php
+++ b/repo/tests/phpunit/includes/WikibaseRepoSchemaTest.php
@@ -12,11 +12,6 @@ use MediaWiki\Tests\Structure\AbstractSchemaTestBase;
  * @license GPL-2.0-or-later
  */
 class WikibaseRepoSchemaTest extends AbstractSchemaTestBase {
-       protected function setUp(): void {
-               parent::setUp();
-               $this->markTestSkipped( 'Temporary disabled for gerrit 1174110' );
-       }
-
        protected static function getSchemasDirectory(): string {
                return __DIR__ . '/../../../sql/abstract/';
        }
lucaswerkmeister-wmde@integration-agent-docker-1040:~/Wikibase$ git log repo/tests/phpunit/includes/WikibaseRepoSchemaTest.php
commit f789f17769846c5c966a2f8d61dc5dc0c41d3b1e
Author: Alexander Vorwerk <zabe@avorwerk.net>
Date:   Wed Jul 30 13:03:53 2025 +0200

    Temporary disable doctrine tests for doctrine update
    
    We need to regenerate some schema files after the upgrade.
    
    Change-Id: I96ec681915404a7058acfc794083f8164e1a7c6d

commit 0dea1809c14e6a9c7365ecbfbf491eb173875730
Author: Daimona Eaytoy <daimona.wiki@gmail.com>
Date:   Sat Dec 14 04:19:45 2024 +0100

    tests: Add structure test for abstract schema
    
    Use the new base class introduced in T381981.
    
    Also regenerate all the schema changes to get rid of drifts, as done in
    I3a3acc0935409d4601eb172664580b13d5fe2340.
    
    Depends-On: I8ae59ce6854922c2804d12986c870d3f0e0c3299
    Bug: T317180
    Change-Id: I4ee25a69017a11c8b489fdbff0441842cccf9543

I don’t know what on earth Git or Zuul have done here, but this sounds like very bad news to me.

There’s one more file where the changes from the commit weren’t applied for some reason, the client/ version of the schema test PHP:

lucaswerkmeister-wmde@integration-agent-docker-1040:~/Wikibase$ git log client/tests/phpunit/integration/includes/WikibaseClientSchemaTest.php
commit f789f17769846c5c966a2f8d61dc5dc0c41d3b1e
Author: Alexander Vorwerk <zabe@avorwerk.net>
Date:   Wed Jul 30 13:03:53 2025 +0200

    Temporary disable doctrine tests for doctrine update
    
    We need to regenerate some schema files after the upgrade.
    
    Change-Id: I96ec681915404a7058acfc794083f8164e1a7c6d

commit 0dea1809c14e6a9c7365ecbfbf491eb173875730
Author: Daimona Eaytoy <daimona.wiki@gmail.com>
Date:   Sat Dec 14 04:19:45 2024 +0100

    tests: Add structure test for abstract schema
    
    Use the new base class introduced in T381981.
    
    Also regenerate all the schema changes to get rid of drifts, as done in
    I3a3acc0935409d4601eb172664580b13d5fe2340.
    
    Depends-On: I8ae59ce6854922c2804d12986c870d3f0e0c3299
    Bug: T317180
    Change-Id: I4ee25a69017a11c8b489fdbff0441842cccf9543
lucaswerkmeister-wmde@integration-agent-docker-1040:~/Wikibase$ grep -B1 -A2 setUp client/tests/phpunit/integration/includes/WikibaseClientSchemaTest.php
class WikibaseClientSchemaTest extends AbstractSchemaTestBase {
	protected function setUp(): void {
		parent::setUp();
		$this->markTestSkipped( 'Temporary disabled for gerrit 1174110' );
	}
lucaswerkmeister-wmde@integration-agent-docker-1040:~/Wikibase$ git show 145631afaf client/tests/phpunit/integration/includes/WikibaseClientSchemaTest.php
commit 145631afafa22050a021c9136b4d56eb56e8c074
Author: Alexander Vorwerk <zabe@avorwerk.net>
Date:   Wed Jul 30 18:47:29 2025 +0200

    Reenable doctrine tests for doctrine update
    
    We regenerate all sql files for this.
    
    This reverts commit bd29e25951bc303895646c08f850ffe812b160bf.
    
    Depends-On: I2bf1093d153bc5ed916045cc73a17df8d3a24150
    Change-Id: I1255dd913eed2c8273f123c4acce947b6d3c03be

diff --git a/client/tests/phpunit/integration/includes/WikibaseClientSchemaTest.php b/client/tests/phpunit/integration/includes/WikibaseClientSchemaTest.php
index a662616cbc..f9024bf49a 100644
--- a/client/tests/phpunit/integration/includes/WikibaseClientSchemaTest.php
+++ b/client/tests/phpunit/integration/includes/WikibaseClientSchemaTest.php
@@ -12,11 +12,6 @@ use MediaWiki\Tests\Structure\AbstractSchemaTestBase;
  * @license GPL-2.0-or-later
  */
 class WikibaseClientSchemaTest extends AbstractSchemaTestBase {
-       protected function setUp(): void {
-               parent::setUp();
-               $this->markTestSkipped( 'Temporary disabled for gerrit 1174110' );
-       }
-
        protected static function getSchemasDirectory(): string {
                return __DIR__ . '/../../../sql/abstract/';
        }

Whereas all the .sql files have their changes applied as expected.

I can reproduce the same tree (still on integration hosts) with a standard Git merge:

lucaswerkmeister-wmde@integration-agent-docker-1040:~/Wikibase$ git show
commit b67e7a07a97f927b3ea410f03322773d81514a8f (HEAD -> T400918)
Merge: eb4a675dc6 145631afaf
Author: Wikimedia Zuul Merger <zuul-merger@contint1002>
Date:   Thu Jul 31 14:52:42 2025 +0000

    Merge commit 'refs/changes/08/1174508/5' of ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase into HEAD

lucaswerkmeister-wmde@integration-agent-docker-1040:~/Wikibase$ git switch -c T400918-2
Switched to a new branch 'T400918-2'
lucaswerkmeister-wmde@integration-agent-docker-1040:~/Wikibase$ git reset --hard eb4a675dc6
HEAD is now at eb4a675dc6 REST: Introduce `SiteLinkGlobalIdentifiersProviderSiteIdsRetriever` service
lucaswerkmeister-wmde@integration-agent-docker-1040:~/Wikibase$ git merge 145631afaf
Merge made by the 'ort' strategy.
 client/sql/mysql/entity_usage.sql                                    |  2 +-
 client/sql/postgres/entity_usage.sql                                 |  2 +-
 client/sql/sqlite/entity_usage.sql                                   |  2 +-
 repo/sql/mysql/archives/patch-wb_changes-change_timestamp.sql        |  4 ++--
 repo/sql/mysql/archives/patch-wb_id_counters-unique-to-pk.sql        |  1 +
 repo/sql/postgres/archives/patch-wb_changes-change_timestamp.sql     |  2 --
 repo/sql/postgres/archives/patch-wb_id_counters-unique-to-pk.sql     |  1 +
 repo/sql/sqlite/archives/patch-wb_changes-change_object_id-index.sql | 27 +++++++++++++++------------
 repo/sql/sqlite/archives/patch-wb_id_counters-unique-to-pk.sql       | 11 +++++++----
 9 files changed, 29 insertions(+), 23 deletions(-)
lucaswerkmeister-wmde@integration-agent-docker-1040:~/Wikibase$ grep -B1 -A2 setUp client/tests/phpunit/integration/includes/WikibaseClientSchemaTest.php
class WikibaseClientSchemaTest extends AbstractSchemaTestBase {
	protected function setUp(): void {
		parent::setUp();
		$this->markTestSkipped( 'Temporary disabled for gerrit 1174110' );
	}
lucaswerkmeister-wmde@integration-agent-docker-1040:~/Wikibase$ git rev-parse @^{tree}
a36c019fff2e2f5c2cdabc3cd39a3e142c22df12

And I just noticed something. In the git log in T400918#11053499, the “Temporary disable doctrine tests for doctrine update” is included on both sides of the merge! Presumably this is why Git produces this bad merge tree. The “left” side of the merge wants to change the PHP files, whereas the “right” side (overall) wants to leave them unmodified (after changing them and then reverting that change); it’s not totally unreasonable for Git to opt for the modified version in this case, even if it’s not what we actually want.

That said, I think the fact that Zuul included that commit on both sides of the merge, and thus ended up testing a tree that is different from what would eventually be merged in Gerrit, must still be a bug? Let’s tag in Zuul.

Change #1175050 abandoned by Lucas Werkmeister (WMDE):

[mediawiki/extensions/Wikibase@master] Reapply "Reenable doctrine tests for doctrine update"

Reason:

no longer necessary, the investigation has progressed past this point

https://gerrit.wikimedia.org/r/1175050

I wonder if this is related to T390719: Change Gerrit default submit strategy to 'Rebase if Necessary' and allowing content merge after all – because without the automatic rebase, I suspect the merge tree would have looked differently. Git could have noticed that the same “Temporary disable doctrine tests…” commit is included on both sides of the tree (merged into master on the “left” side, and as the direct parent of “Reenable doctrine tests…” on the “right” side), and then it would have probably figured out that we wanted the “Reenable…” changes, i.e. reverting the PHP file changes and un-skipping the tests, to be applied on top of that. Whereas due to the rebase, the version of “Temporary disable…” that ended up on the master branch is a different commit (hash) than the direct parent of “Reenable…”, and so it’s not obvious to Git which of them should supersede the other.

Previously, I think you would have had a similar situation only if a to-be-merged change is based on an outdated version of its parent change, and in that case Gerrit (Zuul? Jenkins?) refuses to merge the change even if it’s +2ed.

(Adding to our board to reflect that I spent time on this, though I’m not assigning the task to myself as I think I’ve reached the point where I’d really love to hear from the WMF’s resident CI experts :))

In T400918#11053499, @Lucas_Werkmeister_WMDE wrote:

lucaswerkmeister-wmde@integration-agent-docker-1040:~/Wikibase$ git log --oneline --graph | head -15
*   b67e7a07a9 Merge commit 'refs/changes/08/1174508/5' of ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase into HEAD
|\  
| * 145631afaf Reenable doctrine tests for doctrine update
| * bd29e25951 Temporary disable doctrine tests for doctrine update
* | eb4a675dc6 REST: Introduce `SiteLinkGlobalIdentifiersProviderSiteIdsRetriever` service
* | 098d2f5b9e GQL: Support labels of item values
* | 9f0f4590c6 Add ADR 31 on HTTP specifics in use cases
* | a9efdada36 GQL: Support item values
* | a059d5cdea GQL: Allow filtering by statement property
* | 0c4218b207 Localisation updates from https://translatewiki.net.
* | f789f17769 Temporary disable doctrine tests for doctrine update
* | 217b36b1b3 Align uses of snak / data value HTML
* | 369430e28d GQL: Support novalue and somevalue statements
|/  
* 88005fdfad Localisation updates from https://translatewiki.net.

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/1174431

• bd29e25951 is the pre-merge commit
• f789f17769 is the past-merge commit (due to rebase strategy -> T390719)

So I can agree to lucas question, if this is related to the rebase strategy.

In T399241 this also was a problem for the local clones to determine if the version is merged or not. It seems that cherry-pick is the better way in that case. Not sure if zuul can or should do that as well.

Change #1174770 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Reenable doctrine tests for doctrine update (2nd try)

https://gerrit.wikimedia.org/r/1174770

For the record, the Zuul mergers use git 2.30.2 (from Debian: 1:2.30.2-1+deb11u4) and merge the patch against the branch using the resolve strategy (git merge -s resolve). From the man page:

resolve
This can only resolve two heads (i.e. the current branch and another branch you pulled from) using a 3-way merge algorithm. It tries to carefully detect criss-cross merge ambiguities and is considered generally safe and fast.

The default was changed to ort with Git 2.34.0. Maybe it addresses it.

The merge I did in T400918#11053657 was made by the ort strategy (apparently integration-agent-docker-1040 has git 2.34.1) and resulted in the same (wrong) tree.

lucaswerkmeister-wmde@integration-agent-docker-1040:~/Wikibase$ git log --oneline --graph | head -15
*   b67e7a07a9 Merge commit 'refs/changes/08/1174508/5' of ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase into HEAD
|\  
| * 145631afaf Reenable doctrine tests for doctrine update
| * bd29e25951 Temporary disable doctrine tests for doctrine update
* | eb4a675dc6 REST: Introduce `SiteLinkGlobalIdentifiersProviderSiteIdsRetriever` service
* | 098d2f5b9e GQL: Support labels of item values
* | 9f0f4590c6 Add ADR 31 on HTTP specifics in use cases
* | a9efdada36 GQL: Support item values
* | a059d5cdea GQL: Allow filtering by statement property
* | 0c4218b207 Localisation updates from https://translatewiki.net.
* | f789f17769 Temporary disable doctrine tests for doctrine update
* | 217b36b1b3 Align uses of snak / data value HTML
* | 369430e28d GQL: Support novalue and somevalue statements
|/  
* 88005fdfad Localisation updates from https://translatewiki.net.

To reproduce it:

• set master to the commit disabling the tests: f789f17769
• fetch refs/changes/08/1174508/5 and merge it

git reset --hard f789f17769
git fetch refs/changes/08/1174508/5
git merge FETCH_HEAD

When git processes the merge, it first looks for a common ancestor:

$ git merge-base f789f17769 FETCH_HEAD
88005fdfada9ff10f1c7408a5f3bf92f1a70f221

That is the bottom commit shown in git log --graph.

To do the merge git would compare that merge base against each of the branch then resolves the hunk.

On the master side:

git diff -U0  88005fdfada9ff10f1c7408a5f3bf92f1a70f221 f789f17769 client/tests/phpunit/integration/includes/WikibaseClientSchemaTest.php
diff --git a/client/tests/phpunit/integration/includes/WikibaseClientSchemaTest.php b/client/tests/phpunit/integration/includes/WikibaseClientSchemaTest.php
index f9024bf49a..a662616cbc 100644
--- a/client/tests/phpunit/integration/includes/WikibaseClientSchemaTest.php
+++ b/client/tests/phpunit/integration/includes/WikibaseClientSchemaTest.php
@@ -14,0 +15,5 @@ class WikibaseClientSchemaTest extends AbstractSchemaTestBase {
+       protected function setUp(): void {
+               parent::setUp();
+               $this->markTestSkipped( 'Temporary disabled for gerrit 1174110' );
+       }
+

So that is one hunk that adds the skip on top of the merge base.

For refs/changes/08/1174508/5, well it has two patches that cancel each others and thus there is nothing modified:

$ git diff 88005fdfada9ff10f1c7408a5f3bf92f1a70f221 FETCH_HEAD client/tests/phpunit/integration/includes/WikibaseClientSchemaTest.php
$

git merge then rightfully add the code back since it is missing from the merge base and is not seen as being removed in the branch being merged. Fun times.

At least the code that has entered Gerrit was correct and REBASE_IF_NECESSARY did the job.

Recent Zuul does have support for Rebase https://review.opendev.org/c/zuul/zuul/+/861668 albeit that is only for GitHub.

In T400918#11055027, @hashar wrote:

Recent Zuul does have support for Rebase https://review.opendev.org/c/zuul/zuul/+/861668 albeit that is only for GitHub.

Maybe the cherry-pick mode would be a better fit for our new Gerrit setting?

IMHO, if we can’t ensure Zuul will test the same tree that Gerrit will merge, we should revert the Gerrit config change. This wasn’t some bizarre one-in-a-million accident that we shouldn’t expect to happen again – temporarily disabling a test and then reenabling it later is a relatively common thing for us to do when there are incompatible cross-repo changes.

In T400918#11053499, @Lucas_Werkmeister_WMDE wrote:

Alright, with that and some of the information in mw:Continuous integration/Zuul § Zuul git repositories I was able to get access to the commit (using integration-agent-docker-1040 as an arbitrary host that has the necessary access to contint1002, sorry about that – I’ll try to remember to clean it up later).

FTR, I’ve deleted this Git clone again so it doesn’t “pollute” the integration agent forever. We now know the incorrect merge tree can be reproduced in any Wikibase.git clone using commands similar to:

$ git switch -c T400918 eb4a675dc6
$ git fetch gerrit refs/changes/08/1174508/5
$ git merge 145631afaf
$ grep -B1 -A2 setUp repo/tests/phpunit/includes/WikibaseRepoSchemaTest.php
class WikibaseRepoSchemaTest extends AbstractSchemaTestBase {
        protected function setUp(): void {
                parent::setUp();
                $this->markTestSkipped( 'Temporary disabled for gerrit 1174110' );
        }

I think the following message is also ultimately the same problem, that Zuul doesn’t handle Gerrit’s rebase mode correctly:

Merge Failed.
This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

@Jakob_WMDE had started noticing this a few weeks ago already (example change from around that time), and I just got it again in this change. You can reproduce the broken merge on the command line:

$ # in Wikibase.git
$ git fetch
$ git switch -c tmp-T400918 6bd9b9a7f2 # 6bd9b9a7f2 is current master, specifically the merge-rebased version of “Add a first simple test of the statement view”
Switched to a new branch 'tmp-T400918'
$ git review -d 1176508,8 # the currently-latest patchset of “Set up cypress-axe”
Downloading refs/changes/08/1176508/8 from gerrit
Switched to branch "review/1176508-patch8"
$ git switch tmp-T400918 
Switched to branch 'tmp-T400918'
$ git merge 3ee806dc6d # 3ee806dc6d is the commit we just fetched, i.e. 1176508,8 / the latest patchset of “Set up cypress-axe”
Auto-merging cypress.config.ts
CONFLICT (add/add): Merge conflict in cypress.config.ts
Auto-merging cypress/e2e/wbui2025/viewItem.cy.ts
CONFLICT (add/add): Merge conflict in cypress/e2e/wbui2025/viewItem.cy.ts
Auto-merging cypress/support/e2e.ts
CONFLICT (add/add): Merge conflict in cypress/support/e2e.ts
Auto-merging cypress/tsconfig.json
CONFLICT (add/add): Merge conflict in cypress/tsconfig.json
Auto-merging package-lock.json
CONFLICT (content): Merge conflict in package-lock.json
Auto-merging package.json
CONFLICT (content): Merge conflict in package.json
Recorded preimage for 'cypress.config.ts'
Recorded preimage for 'cypress/e2e/wbui2025/viewItem.cy.ts'
Recorded preimage for 'cypress/support/e2e.ts'
Recorded preimage for 'cypress/tsconfig.json'
Recorded preimage for 'package-lock.json'
Recorded preimage for 'package.json'
Automatic merge failed; fix conflicts and then commit the result.

The change in question (Set up cypress-axe) is based on an earlier chain of changes (Add Cypress tests to Wikibase for wbui2025 testing and Add a first simple test of the statement view); those two changes were rebased on merge but the third change is still based on their un-rebased commits; when Zuul tries to merge it with latest master for the test build, the two parent changes are present on both sides of the merge, and Git fails to do the merge properly.

Other “merge failed” examples:

• https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/1188789

@hashar please consider changing the default Gerrit submit strategy back to merge (revert T390719). At this point, I’m confident in my understanding that Zuul does not support the rebase strategy well, with two known failure modes (and potentially more unknown ones):

1. If, in a chain of changes, a later change reverts changes to a file made by an earlier change, and the earlier change is rebased on submit, then the later change may run in CI with the changes to the file still applied. If the change was to temporarily disable and then reenable a test, then the gate-and-submit build for the later change may still run with the test disabled, allowing Gerrit to merge a broken commit that will break all subsequent CI (this is what happened in the original issue report).
2. If, in a chain of changes, an earlier change adds one or more files and is rebased on submit, the later change will fail with a merge conflict if it was not submitted at the same time, and will be broken until it is rebased on the tip of the branch. (This is what happened in the comment just before this one.)

This will not be fixed by a newer Git version – Zuul is asking the wrong thing of Git in the first place, trying to merge commits without regard for the fact that some of them were already rebased and are now present on both sides of the merge. If we can’t make the Zuul configuration match the Gerrit configuration (because rebase mode is only available for GitHub), I believe we have to change the Gerrit configuration to match the Zuul configuration again (merge on submit).

Apparently the “merge conflict” from T400918#11086095 can happen even when no files were being added, as seen in PS3 of this change.

$ git review -d 1187954,3
Downloading refs/changes/54/1187954/3 from gerrit
Switched to branch "review/1187954-patch3"
$ git merge ffe00e0af8 # current master
Auto-merging repo/includes/AnonymousEditWarningBuilder.php
CONFLICT (content): Merge conflict in repo/includes/AnonymousEditWarningBuilder.php
Auto-merging repo/tests/phpunit/includes/AnonymousEditWarningBuilderTest.php
CONFLICT (content): Merge conflict in repo/tests/phpunit/includes/AnonymousEditWarningBuilderTest.php
Recorded preimage for 'repo/includes/AnonymousEditWarningBuilder.php'
Recorded preimage for 'repo/tests/phpunit/includes/AnonymousEditWarningBuilderTest.php'
Automatic merge failed; fix conflicts and then commit the result.

I guess in this case it happens when both the earlier change and the later change touch the same file? (In addition to, as before, the earlier change having been rebased on submit.)

Thanks for all the digging here @Lucas_Werkmeister_WMDE !

I would like @hashar to definitively answer your whether he'll consider reverting T390719: Change Gerrit default submit strategy to 'Rebase if Necessary' and allowing content merge. @hashar do you see a reason to change that strategy given what happened in this task?

I see your point, @Lucas_Werkmeister_WMDE that having Gerrit's merge strategy be different from Zuul's merge strategy means that in certain edge-cases, the code under test will be different from the code that results in the repository.

But any merge strategy would have been confusing.

Git merges are confusing in cases like this one, there's even a blurb in the git merge man page:

With the strategies that use 3-way merge [...], if a change is made on both branches, but later reverted on one of the branches, that change will be present in the merged result; some people find this behavior confusing.

In this case, Rebase if necessary did what humans (I think) would find intuitive.

Some background digging that may be useful at some point:

• Our zuul uses cGit 2.30.2 where the default merge strategy is recursive. In the current version of zuul, we can set this to resolve or cherry-pick.
• Gerrit uses jGit with supports the recursive or resolve merge strategies
  • Right now, Gerrit defaults to the rebase if necessary strategy
  • the previous merge-if-necessary merge strategy we were using git merge -s recursive (we've been using recursive since 2013)

@Lucas_Werkmeister_WMDE after I investigated, I have considered the issue to be a off-by-one error and an one that is unlikely to surface again and thus did not go any further. Since that is happening, @Tyler and I talked about it today, it seems the easiest is to revert Wikibase to the previous strategy (https://gerrit.wikimedia.org/r/Documentation/config-project-config.html#merge_if_necessary merge if necessary) to make things consistent.

I'll dig further though :-] Our old Zuul can have a different merge strategy applied:

**merge-mode (optional)**
  An optional value that indicates what strategy should be used to
  merge changes to this project.  Supported values are:

  ** merge-resolve **
  Equivalent to 'git merge -s resolve'.  This corresponds closely to
  what Gerrit performs (using JGit) for a project if the "Merge if
  necessary" merge mode is selected and "Automatically resolve
  conflicts" is checked.  This is the default.

  ** merge **
  Equivalent to 'git merge'.

  ** cherry-pick **
  Equivalent to 'git cherry-pick'.

And maybe git cherry-pick would produce the same result as a rebase.

Change #1199018 had a related patch set uploaded (by Hashar; author: Hashar):

[mediawiki/extensions/Wikibase@refs/meta/config] Revert submit strategy back to 'merge if necessary'

https://gerrit.wikimedia.org/r/1199018

Our version of Zuul internally defaults to MERGE_RESOLVE which is calling git merge -s resolve:

try:
    mode = item['merge_mode']
    if mode == zuul.model.MERGER_MERGE:
        commit = repo.merge(item['refspec'])
    elif mode == zuul.model.MERGER_MERGE_RESOLVE:
        commit = repo.merge(item['refspec'], 'resolve')
    elif mode == zuul.model.MERGER_CHERRY_PICK:
        commit = repo.cherryPick(item['refspec'])
    else:
        raise Exception("Unsupported merge mode: %s" % mode)

If in the layout config we set merge-mode: merge, that sets the mode to MERGER_MERGE which does a git merge. The strategy used by cgit 2.30.2 will then be recursive (instead of resolve). Since cgit 2.33.0 the default strategy is ort.

All three merge algorithms (resolve, recursive, ort) are 3-way merge algorithms and as Tyler noticed (T400918#11307758) that is has surprising effect:

if a change is made on both branches, but later reverted on one of the branches, that change will be present in the merged result;

Followed by:

It occurs because only the heads and the merge base are considered when performing a merge, not the individual commits.

And surely one of the Zuul merger instance (contint2002):

*   df7a339f6d Merge commit 'refs/changes/08/1174508/5' of ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase into HEAD
|\  
| * 145631afaf Reenable doctrine tests for doctrine update [1174508/5]
| * bd29e25951 Temporary disable doctrine tests for doctrine update [that is 1174431/2 which has already been merged]
* | f789f17769 Temporary disable doctrine tests for doctrine update [1174431/3]
|/
* 88005fdfad Localisation updates from https://translatewiki.net.

And in that merge commit the test is still skipped:

$ git show df7a339f6:repo/tests/phpunit/includes/WikibaseRepoSchemaTest.php | grep markTestSkipped
		$this->markTestSkipped( 'Temporary disabled for gerrit 1174110' );

Since the 3-way algorithms only consider the merge base and HEADs we have:

• merge base (tests are enabled)
• f789f17769 1174431/3: has markTestSkipped
• 145631afaf 1174508/5: not skipped
• merge result: markTestSkipped

But if I rebase:

$ git checkout 1174508/5
$ git log --oneline --decorate -n3
* 145631afaf (tag: 1174508/5) Reenable doctrine tests for doctrine update
* bd29e25951 (tag: 1174431/2) Temporary disable doctrine tests for doctrine update
* 88005fdfad (tag: merge-base) Localisation updates from https://translatewiki.net.
$ git rebase master # set as f789f17769846c5c966a2f8d61dc5dc0c41d3b1e
warning: skipped previously applied commit bd29e25951
Successfully rebased and updated detached HEAD.
$ grep markTestSkipped repo/tests/phpunit/includes/WikibaseRepoSchemaTest.php
$

The rebase result is the tests are not skipped as a human would have expected. That is because git rebase skipped bd29e25951 which is marking the tests skipped.

So yeah Zuul does not support the rebase algorithm and we'd have to manually rebase when a patchset is obsolete and got reverted.

Yesterday with Tyler I mentioned I previously had to tackle this problem and I found the thread https://lists.zuul-ci.org/pipermail/zuul-discuss/2019-January/thread.html#687 , rereading it, I don't think it addresses the issue of rebasing a chain of patches and the current Zuul cherry-pick mode would pick a single change rather than the chain of dependencies (as I understand it).

The good news is the new Zuul support the rebase strategy http://opendev.org/zuul/zuul/commit/26b9b0e2fb167e27f1ba74f0d3a6542f37c69d55 and we are working on upgrading to it.

Meanwhile we can stick the Wikibase repo to merge if necessary which aligns with what Zuul currently support.

Change #1199018 merged by Hashar:

[mediawiki/extensions/Wikibase@refs/meta/config] Revert submit strategy back to 'merge if necessary'

https://gerrit.wikimedia.org/r/1199018

Mentioned in SAL (#wikimedia-releng) [2025-10-28T10:24:17Z] <hashar> gerrit: changed mediawiki/extensions/Wikibase submit strategy back to "Rebase if necessary" # https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/1199018 | T400918

Mentioned in SAL (#wikimedia-releng) [2025-10-28T10:26:38Z] <hashar> gerrit: changed mediawiki/extensions/Wikibase submit strategy back to "Merge if necessary" # https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikibase/+/1199018 | T400918