Page MenuHomePhabricator

Running parser test files with phpunit.php should behave like running them with parserTests.php
Closed, ResolvedPublic

Description

It seems like when running parser test files with phpunit.php they follow different rules as when running them with parserTests.php.

If I observed this correctly, phpunit.php collects all articles to be created with "!!article", creates them, and then it runs tests. With parserTests.php on the other hand everything is executed in the order it is defined. In some tests it can be important whether a article already exists or not.

There might be other behavioral differences here as well. The whole thing seems incredibly odd to me since there is also some redundant code and the initial globals set up in ParserTest::setupGlobals() are slightly different from globals set up in NewParserTest::setupGlobals().

If there is no good reason against this, both classes, ParserTest and NewParserTest should be reduced to one, or at least one base class/interface. The goal should be that when running phpunit.php parser tests behave exactly like running parserTests.php


Version: 1.20.x
Severity: normal

Details

Reference
bz39473

Event Timeline

bzimport raised the priority of this task from to Normal.Nov 22 2014, 12:57 AM
bzimport set Reference to bz39473.
bzimport added a subscriber: Unknown Object (MLST).
Danwe created this task.Aug 19 2012, 2:52 AM
scfc added a comment.Oct 3 2012, 11:36 PM

It would also be nice to get (very far) away from the current yet-another-syntax and replace parserTests.txt with standard data providers and sane ways (= standard PHP) to set input and check output.

Danwe added a comment.Oct 4 2012, 7:16 PM

Mmh, no strong opinion about that right now. You might want to file a separate bug for this. Even though it is somewhat related. Perhaps this should be discussed on wikitech to see peoples reactions.

Why do we even need parserTests.php? We don't use it in Jenkins, and the PHPUnit tests for the Parser seem good enough (and can be improved if not).

Danwe added a comment.Nov 13 2012, 5:39 PM

Actually, from the current state I would rather prefer to drop phpunit.php being smart and executing parser tests. It seems that this has a somewhat poorer implementation compared with the parserTests.php, parserTests.php seems more flexible to me.
On the other hand, it's nice having them executed automatically with unit tests.

If anyone wants to keep parserTests.php for some reason, we should try to make a common base. And when we have that, we should try to make it more flexible again, stuff like not being able to create upper case articles within those tests and articles being created in the beginning and not in a sequential order really sucks.

(In reply to comment #1)

It would also be nice to get (very far) away from the current
yet-another-syntax and replace parserTests.txt with standard data providers and
sane ways (= standard PHP) to set input and check output.

What is so wrong about using a custom syntax? DSLs are a perfectly respectable way to handle this kind of case. They make tests more concise and easier to write.

(In reply to comment #3)

Why do we even need parserTests.php? We don't use it in Jenkins, and the
PHPUnit tests for the Parser seem good enough (and can be improved if not).

parserTests.php has the following features that PHPUnit does not appear to have (and the majority of them I do not believe are possible to implement within PHPUnit):

  • Better error output; The opening of PHPUnit's failure output includes a pile of garbage, basically an opaque serialized set of arguments before it even gets to the diff. While parserTests is mostly concise. The test name is easy to see. And the diffs are coloured.
  • More control over test output format.
  • Tests can be filtered down to specific files; Useful for working on an extension to restrict to it's own parser tests. In fact you can use parserTests.php on a parser test file before you even register it. Making it very useful for adding tests to an extension for the first time.
  • Tests can be filtered by regexp; Useful to re-run the same set of tests over and over when you are debugging something specific.
  • I haven't tried it yet. But apparently parserTests.php can record to a database and compare against it. From what I understand, useful to check for fixes/regressions.
  • parserTests.php can run disabled tests. Useful to check for tests that were once disabled but happen to have been fixed by some recent commit.
  • parserTests.php runs in around 30s. While the PHPUnit parser tests take about 3m to run (perhaps due to fuzz tests). This can make a really big difference with you running tests over and over during development.

So basically in practice I find that the PHPUnit implementation's one purpose is Jenkins integration. While parserTests is superior in every way for use during actual development.

parserTests.php is the old, stable and fast implementation. I use it locally whenever I review a parser change.

NewParserTest is a a new, unstable, slow implementation with lot of code copy pasted and a full initialization being run on each test. BUT, it uses PHPUnit which is nice for Jenkins.

So we need both for now. My preference would go to make the old parserTests.php to be able to output Junit results which is all what is needed by Jenkins and drop the buggy NewParserTest.

Danwe added a comment.Nov 17 2012, 6:47 PM

I think Daniel F. points are convincing. This is blocking me and improvement of parser testing system. Right now it is simply not possible to write parser tests for certain cases and it seems not possible to implement this in both, PHPUnit parser test testing and parserTests.php.

So, why not teach Jenkins to execute parserTests.php and stop executing them together with unit tests. This would finally free the way to improve parser testing and I could finally release some of the tests for some of my extensions.

brion added a subscriber: brion.Feb 22 2016, 3:09 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 22 2016, 3:09 PM

I think it was a bad idea to make a complex ~1600 LOC integration test system be implemented as a single MediaWikiTestCase derived class. PHPUnit does not add much utility in this case. I think we should have a single implementation which provides an internal API which is used by both entry points.

hashar removed a subscriber: hashar.Jul 23 2016, 10:10 PM

Change 309233 had a related patch set uploaded (by Legoktm):
Refactor parser tests

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

Legoktm assigned this task to tstarling.Sep 9 2016, 8:14 AM

Change 309233 merged by jenkins-bot:
Refactor parser tests

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

TheDJ added a subscriber: TheDJ.Sep 16 2016, 9:38 AM

@tstarling I think this might qualify as my favorite commit of the year. Thank you so much :)