607719 - runtests.py: testconfig system needs overhaul

Status: VERIFIED FIXED

(Tamarin Graveyard :: Tools, defect)

Description

[Chris Peyer]

The testconfig file should be split up into two files:
failconfig = file that tracks all expected failures and skips for tests.  Every entry in this file should have an associated bug number
testconfig = track how and where files should be run.  Bug numbers would not be present in this file

We currently track both in testconfig.

In addition to being able to specify failure modes (see Bug 600661), we should be able to define multiple directives for tests that are all parsed by the runtests framework.

At the moment, runtests will only load the longest matching test/testcase name for each test.

It is also very difficult to specify deep tests, and we are forced to use negative-lookahead regular expressions.  By being able to specify multiple directives for a test, we could just have deep be a directive.

Comment 1

[Chris Peyer]

Attachment: testconfig revamp

Splits out testconfig.txt into two files.

Every test/testcase can have more than one directive specified.

New deep directive added.  Include removed from acceptance  The performance testconfig still supports include.

Comment 2

[Chris Peyer]

Attachment: updated patch with brents email comments

Comment 3

[Brent Baker]

Comment on attachment 495663 [details] [diff] [review]
updated patch with brents email comments

Still working on this patch review but want to capture something that we were working on over IM:

There is a problem with the acceptance performance run is not executing the correct testcases. When the acceptance suite is run with:
./runtests.py --notimecheck --threads=1 -addtoconfig=-performance 

The following testcases should be run:
misc/performance_placeholder
as3/Definitions/Function/RestOptimization
as3/Definitions/Function/ArgumentsOptimization

And the last 2 testcases should be EXCLUDED if any of the following are true about the config:
+ interp
+ wordcode
+ debugger WITHOUT the -Dnodebugger switch

The current regexp does not properly capture this, this pattern however does:
(interp|wordcode|(debugger-(?!.*Dnodebugger)))

While this does capture the testcases properly there is still an issue that the testcase has already been marked as a skip for ALL patterns and excluded for none:
as3/Definitions/Function/RestOptimization, .*, , skip , skip acceptance-perf tests in normal runs

If you change the exclude pattern on this line to ".*performance" then it does work since it will NOT be marked as a skip for any performance run. Chris I will ping you once you get online to discuss the need to have to double mark testcases that you want skipped in normal runs but included into specific "directives"

Comment 4

[Brent Baker]

Comment on attachment 495663 [details] [diff] [review]
updated patch with brents email comments

Some more findings with the new config files:

Looking into why the config 'ppc-mac-tvm-debug-deep' did not run the
same tests with and without the patch I found the following issues:

1) misc/testJitordieSwitch.as 
- listed twice for the deep directive causing it to be run twice.

2) mops/mops.abc_  
- This is running with the new patch when it was not being run on ppc debug before
# bug 505982
mops/mops,^((?!ppc-mac-tvm-debug-).).*deep, include, include slow tests in deep phase
I will look into if this is still valid as soon as a PPC machine is idle

3) spidermonkey/js1_5/Array/regress-157652,, .*, deep, skip slow tests
spidermonkey/js1_5/Regress/regress-203278-1,, .*, deep, skip slow tests
spidermonkey/js1_5/Regress/regress-360969-0(5|6),, .*, deep, skip slow tests
I think this is just a mistake that the include and exclude patterns are reveresed, include pattern should be ".*"

4) The following have double entries, I think there are double entries in the original, but should clean it up
ecma3/Unicode/u9000_CJKUnifiedIdeographs,.*,, ats_skip, bug 499685 skip slow tests
ecma3/Unicode/u9000_CJKUnifiedIdeographs,.*deep, include, include slow tests in deep phase

Comment 5

[Brent Baker]

Comment on attachment 495663 [details] [diff] [review]
updated patch with brents email comments

Chris is working on an update to this patch based on comments and IM conversations.

Comment 6

[Brent Baker]

(In reply to comment #4)
> 2) mops/mops.abc_  
> - This is running with the new patch when it was not being run on ppc debug
> before
> # bug 505982
> mops/mops,^((?!ppc-mac-tvm-debug-).).*deep, include, include slow tests in deep
> phase
> I will look into if this is still valid as soon as a PPC machine is idle
> 

I ran this test multiple times on a PPC machine and did not see the assertion so the removal of the specific skip on ppc-mac-tvm-debug is fine.

Comment 7

[Chris Peyer]

Attachment: Updated patch to address comments

Addresses issues in comments 3 and 4.

re comment 3:
Tests now should never have to be specified twice in the config files.  When a test has its directive set to either deep or performance, it will be excluded from regular test runs without an explicit skip.

re comment 4:
1 & 4: removed the double listings for testJitordieSwitch and u9000.  Also added logic to runtestBase to not run a test twice even if it is listed twice.

2: removed the deep skip and verified that the test only runs in deep mode for ppc mac debug

3. fixed the typos so the commas are in the right place

This patch was rebased to redux tip (changeset 5725 aba315ca64bc).

Comment 8

[Brent Baker]

Comment on attachment 501525 [details] [diff] [review]
Updated patch to address comments

1) failconfig.txt:
Make sure to apply this diff before pushing:
http://hg.mozilla.org/tamarin-redux/diff/9c0aeb405ad3/test/acceptance/testconfig.txt

2) testconfig.txt:
Has the include/exclude patterns wrong for this entry:
spidermonkey/js1_5/Regress/regress-360969-0(5|6),, .*, deep, skip slow tests
should be:
spidermonkey/js1_5/Regress/regress-360969-0(5|6), .*,, deep, skip slow tests

3) ecma3/Unicode section that marks the tests for the deep phase:
I think there is a subtle change here that should be cleaned up. Previously the tests were marked to be skipped if the config was -Dgreedy, now they are being marked for the "deep" directive when -Dgreedy is defined. This currently works as expected since the -Dgreedy is NOT run with a deep directive, so they will never be run when -Dgreedy is used, BUT this is not clear. I suggest that a new "-Dgreedy" block be created in the config file and clearly mark these tests to be skipped.

4) misc/testJitordieSwitch is still listed twice in testconfig.txt

Please push this through the sandbox to get additional coverage

I do not see a need for further review after these changes are made

Comment 9

[Tamarin Bot]

changeset: 5759:a4368038bf89
user:      Chris Peyer <[e-mail]>
summary:   Bug 607719: revamp the testconfig setup (r=brbaker)

http://hg.mozilla.org/tamarin-redux/rev/a4368038bf89

Comment 10

[Tamarin Bot]

changeset: 5761:3f09c8e2816b
user:      Chris Peyer <[e-mail]>
summary:   Bug 607719 followup: deep tests that do not match the current config will run in regular mode, but performance tests only run when in acceptance-performance mode (r=cpeyer)

http://hg.mozilla.org/tamarin-redux/rev/3f09c8e2816b

Comment 11

[Tamarin Bot]

changeset: 5762:18e04aa177ba
user:      Chris Peyer <[e-mail]>
summary:   Bug 607719: missed removing a line when merging with rev 5712 (r=cpeyer)

http://hg.mozilla.org/tamarin-redux/rev/18e04aa177ba

Comment 12

[Brent Baker]

Attachment: check_out_file() needs to have access to the testName

Comment 13

[Brent Baker]

Chris there are issues with the current patches submitted to tamarin. It appears that the code that you had to remove files from running in a normal acceptance run if it is defined for a deep phase is not functioning.

There are a handful of tests that are marked for deep phase only on arm (Unicode tests) that are currently being executed.

Also there is an issue with testJitorDie testcase running on arm-linux when it is not supposed to be. Not sure if it is an issue with the fact that the testcase is marked to be run in deep with ".*" but is excluded for arm... does that mean then that it is included in a normal run when running on ARM?

Did this all get run through the sandbox prior to being pushed into tr?

Comment 14

[Brent Baker]

Attachment: check_out_file() needs testName, fix test removal

check_test_out() requires the testName to be passed to it

BUG: While removing tests that are set to run in a custom directive the list of tests are iterated over and then if is supposed to run in a custom directive the test is removed from the list. Problem is that this is done while iterating over the list, which does not guaranteed to iterate over the correct items any more. Instead keep track of the tests to remove and then after the complete list of tests to remove is generated, remove them in their own loop.

This issue can be seen by running the ecam3/Unicode/ suite, if you run with a config string of "arm-lnx" you should only be running 99 test files, however currently you will run 103 since some of the tests cases will be run even though they are marked to be run in the deep phase of the build.

Comment 15

[Tamarin Bot]

changeset: 5772:fbb6f4bfdb34
user:      Brent Baker <[e-mail]>
summary:   Bug 607719: fix how testcases are removed that run in the deep phase of the build. Also fix an issue with check_out_file(). Included in this patch is a workaround for the testJitordieSwitch still running on arm-lnx in the deep phase when it is not supposed to (r+cpeyer)

http://hg.mozilla.org/tamarin-redux/rev/fbb6f4bfdb34