Page MenuHomePhabricator

ApiPrefixUniquenessTest::testPrefixes fails because linterstats and languagestats are both using "ls"
Closed, ResolvedPublic

Description

https://integration.wikimedia.org/ci/job/quibble-vendor-mysql-php74-noselenium-docker/57590/console

16:00:17 1) ApiPrefixUniquenessTest::testPrefixes
16:00:17 Module prefix 'ls' is shared between MediaWiki\Extension\Translate\Statistics\QueryLanguageStatsActionApi and MediaWiki\Linter\ApiQueryLinterStats
16:00:17 
16:00:17 /workspace/src/tests/phpunit/structure/ApiPrefixUniquenessTest.php:29
16:00:17 /workspace/src/tests/phpunit/MediaWikiIntegrationTestCase.php:516
16:00:17 
16:00:17 FAILURES!
16:00:17 Tests: 9994, Assertions: 37561, Failures: 1, Skipped: 77.

Not clear why this is suddenly a problem.

Event Timeline

Seems to be https://gerrit.wikimedia.org/r/c/integration/config/+/833061: It adds DiscussionTools as a dependency to VisualEditor. Translate depends on VisualEditor and DiscussionTools depend on Linter.

Change 837004 had a related patch set uploaded (by Jforrester; author: Jforrester):

[integration/config@master] Revert "Add DiscussionTools as a dependency for VE"

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

Change 837683 had a related patch set uploaded (by MSantos; author: MSantos):

[mediawiki/core@master] tests: suppress API prefix uniqueness check for 'ls'

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

Change 837004 merged by jenkins-bot:

[integration/config@master] Revert "Add DiscussionTools as a dependency for VE"

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

@Jdforrester-WMF a side note, we need DiscussionTools tests to run with VE to avoid other possible breakages. Not having those is a "soft" block on VE tests cc/ @daniel @xSavitar

@Jdforrester-WMF a side note, we need DiscussionTools tests to run with VE to avoid other possible breakages. Not having those is a "soft" block on VE tests cc/ @daniel @xSavitar

Then it sounds like you have a blocker of getting your team's code in the Linter extension to be compatible with the Translate extension.

Change 837693 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/extensions/Linter@master] ApiQueryLinterStats: Change the 'ls' prefix to 'linters' to avoid conflict

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

Change 837693 merged by jenkins-bot:

[mediawiki/extensions/Linter@master] ApiQueryLinterStats: Change the 'ls' prefix to 'lntrst' to avoid conflict

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

Change 837706 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/extensions/Linter@REL1_39] ApiQueryLinterStats: Change the 'ls' prefix to 'lntrst' to avoid conflict

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

Change 837707 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/extensions/Linter@REL1_38] ApiQueryLinterStats: Change the 'ls' prefix to 'lntrst' to avoid conflict

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

Change 837708 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/extensions/Linter@REL1_37] ApiQueryLinterStats: Change the 'ls' prefix to 'lntrst' to avoid conflict

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

Change 837709 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/extensions/Linter@REL1_35] ApiQueryLinterStats: Change the 'ls' prefix to 'lntrst' to avoid conflict

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

I've cherry-picked this to all release branches, but not to the deployment branches. If Translate needs a back-port to wmf.3 in the next couple of days, we'll need to also cherry-pick and deploy that change there too.

Change 837706 merged by jenkins-bot:

[mediawiki/extensions/Linter@REL1_39] ApiQueryLinterStats: Change the 'ls' prefix to 'lntrst' to avoid conflict

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

Change 837707 merged by jenkins-bot:

[mediawiki/extensions/Linter@REL1_38] ApiQueryLinterStats: Change the 'ls' prefix to 'lntrst' to avoid conflict

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

Change 837708 merged by jenkins-bot:

[mediawiki/extensions/Linter@REL1_37] ApiQueryLinterStats: Change the 'ls' prefix to 'lntrst' to avoid conflict

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

Change 837709 merged by jenkins-bot:

[mediawiki/extensions/Linter@REL1_35] ApiQueryLinterStats: Change the 'ls' prefix to 'lntrst' to avoid conflict

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

Legoktm renamed this task from ApiPrefixUniquenessTest::testPrefixes to ApiPrefixUniquenessTest::testPrefixes fails because linterstats and languagestats are both using "ls".Oct 3 2022, 4:23 PM

Change 837710 had a related patch set uploaded (by Jforrester; author: Jforrester):

[integration/config@master] Zuul: [mediawiki/extensions/VisualEditor] Add DiscussionTools dependency [re-try]

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

Nikerabbit lowered the priority of this task from Unbreak Now! to High.Oct 4 2022, 7:10 AM

Thanks to everyone for the help!

Change 837683 abandoned by Nikerabbit:

[mediawiki/core@master] tests: suppress API prefix uniqueness check for 'ls'

Reason:

Handled by changing prefix in the Linter extension

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

What remains to be done here?

I guess we left this open in case anyone was actually using the Linter stats via a bot and wanted a venue to complain. Definitely fixed now. Do we want to re-add the cyclical dependency as in https://gerrit.wikimedia.org/r/c/integration/config/+/837710 ?

Do we want to re-add the cyclical dependency as in https://gerrit.wikimedia.org/r/c/integration/config/+/837710 ?

Does that actually do anything? VisualEditor tests don't need DiscussionTools to be installed to run.

Does that actually do anything? VisualEditor tests don't need DiscussionTools to be installed to run.

Changes to VE can break DT, so DT tests should run for every patch against VE.

Oh, it wasn't clear to me that this would do that. I thought that adding it to dependencies would just install it, but not also run its tests. But it turns out it does (the jobs just run the whole test suite, not the tests from a specific extension).

Oh, it wasn't clear to me that this would do that. I thought that adding it to dependencies would just install it, but not also run its tests. But it turns out it does (the jobs just run the whole test suite, not the tests from a specific extension).

I wish we could specify which kind of dependency it is - "needs to run" or "may break". We could skip the tests of stuff that is just needed to run.

It doesn't seem like anyone wants to do anything else about this.