Last modified: 2013-10-30 13:17:20 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T41473, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 39473 - Running parser test files with phpunit.php should behave like running them with parserTests.php
Running parser test files with phpunit.php should behave like running them wi...
Status: NEW
Product: MediaWiki
Classification: Unclassified
Unit tests (Other open bugs)
1.20.x
All All
: Normal normal (vote)
: ---
Assigned To: Nobody - You can work on this!
:
Depends on:
Blocks: 39461
  Show dependency treegraph
 
Reported: 2012-08-19 02:52 UTC by Daniel A. R. Werner
Modified: 2013-10-30 13:17 UTC (History)
6 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Daniel A. R. Werner 2012-08-19 02:52:12 UTC
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
Comment 1 Tim Landscheidt 2012-10-03 23:36:43 UTC
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.
Comment 2 Daniel A. R. Werner 2012-10-04 19:16:17 UTC
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.
Comment 3 Krinkle 2012-11-12 13:01:54 UTC
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).
Comment 4 Krinkle 2012-11-12 13:02:10 UTC
See also bug 41989.
Comment 5 Daniel A. R. Werner 2012-11-13 17:39:53 UTC
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.
Comment 6 Daniel Friesen 2012-11-16 04:14:01 UTC
(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.
Comment 7 Antoine "hashar" Musso (WMF) 2012-11-17 09:13:01 UTC
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.
Comment 8 Daniel A. R. Werner 2012-11-17 18:47:19 UTC
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.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links