Page MenuHomePhabricator

Delete test leftovers from CI
Closed, InvalidPublic

Description

While writing phpunit tests in this patch I tried to create a user and write to a page inside a data provider. However, I just realized that writes inside data providers happen on the actual DB. As a POC, see the failure for PS3: the user created while testing PS2 still existed in the DB. Also, PS7 fixes the issue and passes localy, but still fails because the user hasn't been deleted. Thus, I'm asking if someone could please delete such leftovers (basically, a user named "AbuseFilter throttled user" and a page named "AF - Raising edit count").

Event Timeline

The CI job wmf-quibble-vendor-mysql-hhvm-docker uses Quibble (a wrapper to run MediaWiki tests). One of Quibble feature is that it initializes and spawns a fresh and empty database. MediaWiki is then installed against that and the test runs. There is thus no shared state between the build. They all start with a fresh environment.

PHPUnit executes code in the data provider BEFORE running any other tests, which might have consequence for your test.

The test case for editcount does five edits and is hardcoded to expect editcount-5 as an identifier. But that does not take in account that other tests triggered the code which has raised the identifier. The test should look at the current id, do the 5 edits then expects current id + 5.

Last note, AbuseFilter is deployed on Wikimedia production and the CI job runs tests with a few other extensions. Their tests might interact in some way with the state of the database.

@hashar Thanks for the comment! A few considerations:

  • The 5 edits are now being made inside the actual test method, which is executed at the right time, and only once; so the test should in theory succeed now.
  • Checking the current count and expecting a +5 is indeed a good idea; I'll implement it after having rewritten the method to have a better signature
  • I don't think other extensions should interact with this one, but I'm not excluding it of course

However, I still doubt the fresh DB thing. CI logs have been deleted since then, but I still remember that any time I submitted a new patchset, the actual editcount would increase by 5, just like it did locally. It took me a while to figure out why, until I found the test user in the actual user table. I find it hard to believe, but it really looks like some data is persisting.

I can guarantee there is no persisting data. The tests are run in a Docker container, the database is created in a temporary directory which is a temporary filesystem (tmpfs). Once the tests have been completed, the database is stopped and the tmpfs destroyed entirely. The next build would use fresh tmpfs as well and most probably run on a different host.

If you have an expected test user, it must have been introduced by a data provider or another tests that have run previously. Potentially due to one of the other extensions for which tests are running as well :-). It is hard to figure out the root cause though.

Change 479702 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] [DNM] CI test

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

@hashar I submitted the patch above to provide some sort of POC. This is what I did:

  • PS1: makes 5 edits inside the data provider; the test was NOT inside the Database group => CI passed (edit count === 5). Also did a recheck but aborted it.
  • PS2: same as PS1, but inside the Database group; CI failed because it Failed asserting that 5 is identical to 10. Same result with a recheck.

The edit count being 10 in PS2 tests makes me think that something is persisting. I cannot say what, or how, but I cannot see another explanation.

greg subscribed.

There is nothing persisting, as @hashar said. If it were, wouldn't PS3 result in 15 instead of 10? It is, however, showing 10 instead.

PS1 has this as the output: 3832, Assertions: 12770, Skipped: 104
PS2 and PS3 have this instead: Tests: 3835, Assertions: 12773, Failures: 1, Skipped: 104.

It's running a different set of tests for some reason. There is no persisting data. This is a test mis-configuration or mis-definition.

If it were, wouldn't PS3 result in 15 instead of 10?

True. I retried and still got the same result. This is still strange though, because in theory it should just be 5...

Change 479702 abandoned by Daimona Eaytoy:
[DNM] CI test

Reason:
Done

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