Page MenuHomePhabricator

parserTests has 14 tests that have failed from the start, which confuses testing results
Closed, ResolvedPublic

Description

Author: dnessett

Description:
parserTests.txt includes 14 tests that have never worked. This confuses testing, especially when developers need to determine whether their feature or bug-fix work introduces bugs into Mediawiki. The situation needs to be resolved to improve the Mediawiki Testing Environment.

Attached are 7 patches that may be used in various combinations to resolve this bug. These patches have been merged, diffed and tested against r53808. The patches modify parserTests.php, parserTests.inc and parserTests.txt in the following way:

+ parserTestsKTFRemoved.txt.patch comments out those tests (14 in total) that have failed since they were added to the test set. This patch should be applied against parserTests.txt.

+ parserTestsKTFDisabled.txt.patch adds the disabled option to those tests (14 in total) that have failed since they were added to the test set. This patch should be applied against parserTests.txt.

+ parserTestsRunDisabled.inc.patch and parserTestsRunDisabled.php.patch add the --run-disabled option to parserTests. When run with this option specified, all tests with the disabled option are included in those run. These patches should be applied against parserTests.inc and parserTests.php respectively.

+ parsertTestsKTF.inc.patch adds a test option, knowntofail. A test including this option returns the status of KNOWN_TO_FAIL when the test fails and FAILED! when the test succeeds. It also implements the --ktf-in-fail application option (which is noted as an option to parserTests by parserTestsKTF.php.patch -- see below). When this application option is set, tests with the knowntofail option accumulate against failure statistics. When this application option is missing, tests with the knowntofail option accumulate against success statistics and the number of tests with KNOWN_TO_FAIL status is reported. This patch should be applied against parserTests.inc.

+ parserTestsKTF.txt.patch adds the knowntofail option to those tests (14 in total) that have always failed. This patch should be applied against parserTests.txt.

+ parserTestsKTF.php.patch adds --ktf-in-fail to the list of noted application options (i.e., those returned from parserTests --help). When parserTests is run with this option all knowntofail tests that fail accumulate against the failure statistics (failure count and failure percentage). This patch should be applied against parserTests.php.

Combinations of these patches provide the following options:

+ Install none of these patches and do nothing else:

+ Pros: parserTests behavior remains as currently defined.
+ Cons: parserTests behavior remains as currently defined.

+ Install none of these patches and define successful output as that which the Parser currently returns (this would require changing parserTests.txt using a patch not provided here):

+ Pros: parserTests is brought into compliance with the test set.
+ Cons: It is generally bad practice to define success without understanding the logic behind it.

+ Install none of these patches, file bugs against the 14 failing tests and then fix the bugs:

+ Pros: parserTests is brought into compliance with the test set.
+ Cons: It has been suggested that the parserTests logic is sufficiently complex that it may be difficult to modify it to pass the known to fail tests.

+ File bugs against the 14 failing tests and patch parserTests.txt with parserTestsKTFRemoved.txt.patch:

+ Pros: Testing the Parser using parserTests does not return confusing test results. Developers can concentrate on those tests that fail due to the addition of new functionality or intefering bug fixes.
+ Cons: Since the known to fail tests no longer report their presence, it may be easy for the community to forget they are still a problem.

+ Patch parserTests.txt with parserTestsKTFDisabled.txt.patch. Optionally file bugs against the known to fail tests:

+ Pros: Testing the Parser using parserTests does not return confusing test results. Developers can concentrate on those tests that fail due to the addition of new functionality or intefering bug fixes.
+ Cons: There are other disabled tests in parserTests.txt. These could become confused with the known to fail tests.

+ Patch parserTests|.inc|.php with parserTestsRunDisabled|.inc.patch|.php.patch:

+ Pros: This would allow disabled known to fail tests to run without editing parserTests.txt
+ Cons: Specifciation of this option runs all disabled tests, not just those known to fail. Test results using this option might confuse developers.

+ Patch parserTests|.inc|.txt with parserTestsKTF|.inc.patch|.php.patch:

+ Pros: parserTests output and summary statistics indicate which tests succeeded, failed and returned known-to-fail status.
+ Cons: Creates a new test status for what should be a temporary problem. This is a questionable software architecture decision.

+ Patch parserTests.php with parserTestsKTF.php:

+ Pros: those of the previous option. Adds the ability to accumulate ktf failure status in failure statistics.
+ Cons: those of the previous option. Raises the question of what is the proper statistics strategy when the --ktf-in-fail option is missing (on runs with this option missing, known-to-fail tests accumulate against success statistics).

The provided patches do not address whether the 'testrun' and 'testitem' tables should be modifed to note the specification of application options that change the statistics (i.e., either --ktf-in-fail or --run-disabled) or the use of the knowntofail option for a particular test. These tables are not modified by the patches.


Version: unspecified
Severity: enhancement

Details

Reference
bz19957

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 10:39 PM
bzimport set Reference to bz19957.

dnessett wrote:

tar file with 7 patches referenced in bug description

Attached:

Can't you just make it a unified diff and attach it as a patch?

dnessett wrote:

(In reply to comment #2)

Can't you just make it a unified diff and attach it as a patch?

The patches are organized into 3 groups that are somewhat mutually exclusive (at least in intent). In addition, the options specified use various combinations of the patches. So, I think leaving them separate is best.

dnessett wrote:

There is a typo in the second to last option. It should read:

+ Patch parserTests|.inc|.txt with parserTestsKTF|.inc.patch|.txt.patch:

Committed the disabling patch & run-disabled option in r53919. Thanks for the patches!

Note that one of the test cases disabled in the posted patch works for me, so I've un-disabled it.