Page MenuHomePhabricator

Flaky tests ParserOutputAccessTest::testOldRevisionCacheSplit and ::testOldRevisionUseCached
Closed, ResolvedPublic

Description

There is a flaky test in core, which makes the merge of https://gerrit.wikimedia.org/r/c/mediawiki/core/+/653551 a bit complicated

It fails on postgres, but also on php7.3

There was 1 failure:

1) ParserOutputAccessTest::testOldRevisionCacheSplit
French output should be in the cache
Failed asserting that null is not null.

/workspace/src/tests/phpunit/includes/page/ParserOutputAccessTest.php:532
/workspace/src/tests/phpunit/MediaWikiIntegrationTestCase.php:437
/workspace/src/maintenance/doMaintenance.php:106

and also

ParserOutputAccessTest::testOldRevisionUseCached
Expectation failed for method name is "getRenderedRevision" when invoked at most 1 times.
Expected invocation at most 1 times but it occurred 2 time(s).

/workspace/src/tests/phpunit/MediaWikiIntegrationTestCase.php:446
/workspace/src/maintenance/doMaintenance.php:106

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 4 2021, 12:58 AM
holger.knust triaged this task as Medium priority.Jan 12 2021, 2:25 PM
holger.knust added subscribers: Pchelolo, daniel.
Ammarpad added a subscriber: Ammarpad.EditedJan 16 2021, 6:38 AM

Another test in the same file is also failing on postgres

06:22:18 There was 1 failure:
06:22:18 
06:22:18 1) ParserOutputAccessTest::testOldRevisionUseCached
06:22:18 Expectation failed for method name is "getRenderedRevision" when invoked at most 1 times.
06:22:18 Expected invocation at most 1 times but it occurred 2 time(s).
Pchelolo assigned this task to daniel.Jan 16 2021, 5:34 PM

Gonna boldly assign it to Daniel as the original author of that test suite

DannyS712 renamed this task from Flaky test ParserOutputAccessTest::testOldRevisionCacheSplit to Flaky tests ParserOutputAccessTest::testOldRevisionCacheSplit and ::testOldRevisionUseCached.Jan 16 2021, 11:37 PM
DannyS712 updated the task description. (Show Details)

Again, happenning repeatedly

20:02:37 There was 1 failure:
20:02:37 
20:02:37 1) ParserOutputAccessTest::testOldRevisionCacheSplit
20:02:37 French output should be in the cache
20:02:37 Failed asserting that null is not null.
Ladsgroup added a subscriber: Ladsgroup.
Ammarpad added a comment.EditedMon, Feb 1, 7:28 PM

This is still happening randomly, failing unrelated patches during merge.
https://integration.wikimedia.org/ci/job/mediawiki-quibble-vendor-mysql-php74-docker/4329/console

Change 660912 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/core@master] Mark broken ParserOutputAccess tests as broken

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

I don't know if it's fine to disable it. It seems like a rather important test.

The fact that's it's flaky at this rate shows there's logic problem with the test or the code it's testing. Since there's no fix upto now I don't see any harm in halting it, it can simply be reenabled anytime the issue is fixed. But leaving it hoping the issue to simply go away is wasting people's time.
There are many tests that are marked broken in core, I don't think all of them are marked because they're not important. It's simply because they're broken and no one has proposed a fix for them just like this one.

daniel added a comment.Tue, Feb 2, 4:04 PM

The fact that's it's flaky at this rate shows there's logic problem with the test or the code it's testing. Since there's no fix upto now I don't see any harm in halting it, it can simply be reenabled anytime the issue is fixed. But leaving it hoping the issue to simply go away is wasting people's time.

If the test is disabled, there is no incentive to fix it.

Personally, my problem with investigating is that it's random, and only on PostGres. That makes it hard to investigate, I don't know where to start.

There are many tests that are marked broken in core, I don't think all of them are marked because they're not important. It's simply because they're broken and no one has proposed a fix for them just like this one.

Nobody ever looks at them again. That's why there are many.

It not fail on postgres only, the task description also mention php7.3 which is running with mysql (that refers to job mediawiki-quibble-vendor-mysql-php73-docker). From my understanding there is no postgres explicit with php7.3

daniel added a comment.Tue, Feb 2, 8:24 PM

Hm, the error message indicates that something is dropped from the fake parser cache used by the tests. Which shouldn't be possible, since it's backed by a HashBagOStuff. Unless a very low expiry time is set, and the test is running very slowly. But the expiry time should be one hour, I can't imagine it running that slowly...

daniel added a comment.Wed, Feb 3, 9:49 PM

Ok, I was able to reproduce this by inserting sleep(1) between the two calls to getParserOutput() in testOldRevisionUseCached(). Seems like something is a bit too eager to expire...

Change 661513 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Set proper epoch in cache tests.

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

Change 661513 merged by jenkins-bot:
[mediawiki/core@master] Set proper epoch in cache tests.

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

Change 660912 abandoned by Ammarpad:
[mediawiki/core@master] Mark broken ParserOutputAccess tests as broken

Reason:

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

daniel closed this task as Resolved.Wed, Feb 10, 10:17 AM