Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dmandelin, Assigned: dmandelin)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(3 attachments, 12 obsolete attachments)

20.88 KB, patch
Details | Diff | Splinter Review
49.52 KB, patch
Details | Diff | Splinter Review
1.39 KB, text/plain
Details
(Assignee)

Description

9 years ago
Created attachment 393687 [details] [diff] [review]
Patch

This is a new harness for jstests for people to evaluate. I've been using a more primitive version for a while with good results. This harness has less flexibility than jsDriver.pl and may or may not be suitable for heavy test duty. But it has these features, which I really like for my day-to-day developer testing

- Simple command line. Basic usage is

    python3.1 jstests.py <path-to-js>

There are a few options, mostly to control output. You can also run a single test just by giving a substring of the test name (Actually, it runs all the tests with that substring, but if you give a bug number, you'll basically get what you want.) Here's how to do it and print out the command lines run and output:

    python3.1 -so jstests.py <path-to-js> <test-substring>

- Runs tests in parallel on any number of worker threads. I can finish an entire run in 5-6 minutes on 2 processors this way.

- Show results so far in a simple progress bar format:

    [ 282|   0|   5] 32% =================>                | 61.6s

The numbers in brackets on the left are Mochitest-style: (#pass, #unexpected fail, #fail). This way you can always easily see how far along you are and whether you have failed any tests yet.

- Show final results in a simple fixed/regressed format. I created a file of known failures and known crashers based on current tip. If you have no differences from that known set, the entire output (except for progress bar and any extras you turn on) will be:

    PASS

Otherwise, you will just get a list of tests that have regressed, and a list of test that you have fixed, if any.
(Assignee)

Comment 1

9 years ago
Created attachment 393694 [details] [diff] [review]
Patch with proper progressbar.py

The original patch was using a symlink to a file that is not in the tree (yet).
Attachment #393687 - Attachment is obsolete: true
(Assignee)

Comment 2

9 years ago
Created attachment 394365 [details] [diff] [review]
Patch 2

danderson and I have been using this for a few days and we like it OK. It only adds a new harness, no deletions or changes of existing stuff.
Assignee: general → dmandelin
Attachment #393694 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #394365 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #394365 - Flags: review? → review?(bclary)
(Assignee)

Comment 3

9 years ago
Created attachment 395979 [details] [diff] [review]
Patch 3

This version adds support for bc's 'jstests.list' manifests--if the '-m' option is given, they are used as the baseline (giving which tests are expected to fail, should not be run, etc).

bc (and anyone else interested), a few questions came up:

1. Exactly what do 'fail' and 'skip' mean? I guessed that 'fail' means a test is expected to fail (i.e., print 'FAILED'), and that 'skip' means a test is expected to crash (i.e., return nonzero exit code). But when I inspected the results, this seems not to be quite right:

  (a) Many 'fail' tests return a nonzero exit code. As the script is now, this
      gets printed as a regression (any "error" behavior that is not the same
      as the baseline is considered a regression by my script). So it looks like
      'fail' means either a 'FAILED' printout or a nonzero exit code. Which
      would be fine; I just need to know.

  (b) On the other hand, it looks like 'skip' tests often print 'FAILED' but
      return zero. In a shell testing environment I assumed that it would be
      fine to crash, so I run all 'skip' tests and allow them to crash. This way
      it is detected if they are fixed. But perhaps you really want 'skip' tests
      not to run, even in shell?

2. My output format is for dev testing: you get a progress bar while it runs, and a summary report at the end. Do we want an output format that prints things like 'TEST-PASS' and 'TEST-UNEXPECTED-FAIL'?

3. What should be done for tests listed in the manifest that are not found? And also for test files in the dirs that are not listed in the manifest?
Attachment #394365 - Attachment is obsolete: true
Attachment #394365 - Flags: review?(bclary)

Comment 4

9 years ago
(In reply to comment #3)

If at all possible I would like to mimic the output strategy of the reftest extension: http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest.js

> Created an attachment (id=395979) [details]
> Patch 3

I'll look over the patch and comment separately.

> 
> 
> 1. Exactly what do 'fail' and 'skip' mean? I guessed that 'fail' means a test
> is expected to fail (i.e., print 'FAILED'), and that 'skip' means a test is
> expected to crash (i.e., return nonzero exit code). But when I inspected the
> results, this seems not to be quite right:
> 

"fails"' and "skip"'s meaning can be understood in their original context from the browser based reftests and js tests where where the tests are run inside a single invocation of the browser. 

skip:
A test which crashes, terminates early, or which runs over an allotted time is marked "skip" so that it does not cause the entire browser test run to terminate prematurely or fail to complete due to a hang or long running test.

Crashing or terminating prematurely is not an issue with the shell based tests where the shell is started and stopped for each test although long running tests would still be marked as "skip". Tests which are obsolete would be marked as "skip" as well. The keyword "skip" will remove the need for the ignored test lists. Tests which are skipped produce TEST-KNOWN-FAIL testid (SKIP) messages. 

fails:
"fails" means that the test is expected to fail but not crash. As you noted this is problematic for the shell the distinction between a "normal" non-zero exit code and a real crash are less clear and where some tests pass when they have non-zero exit codes. For example, the -n tests pass when they have a non-zero exit code (3). Other tests signify they expect a non-zero exit code using one or more calls to expectExitCode(n) which outputs the string 

--- NOTE: IN THIS TESTCASE, WE EXPECT EXIT CODE n ---

Tests which expect non-zero exit codes should be treated as passing if they return one of the expected exit codes.

It is impossible on Windows to distinguish when a script terminates due to a JavaScript error with exit code 3 from the case where the script terminated due to a fatal JavaScript Assertion which also has exit code 3. Note fatal JavaScript Assertions on Windows do not print an "Assertion failure:" message.

My personal preference is to not introduce new tests which can not have passing non-zero exit codes, at least not exit code 3.

For tests marked with "fails", the output behavior depends on the number of subtests. In the case where a test has only one subtest, a failure can be marked with TEST-KNOWN-FAIL and a pass can be marked with TEST-UNEXPECTED-PASS. In the case where a test may have several subtests some which pass and some which fail, it was decided in the browser based reftests that subtests which failed would still be marked as TEST-KNOWN-FAIL but that subtests which passed would be marked as TEST-PASS(EXPECTED RANDOM). This is because we don't have a way in this manifest approach of flagging the individual subtest's pass or fail status.

On a related issue, I consider a test which has a zero exit code but which does not output a test result to be a failure. 

To turn the free-form format used in the original JavaScript tests into something a bit more understandable I include js-test-driver-end.js after the test file which calls jsTestDriverEnd() to perform some clean up and output a formatted list of test results. This gives good data on the individual subtests and also whether a test really completed normally.

>   (a) Many 'fail' tests return a nonzero exit code. As the script is now, this
>       gets printed as a regression (any "error" behavior that is not the same
>       as the baseline is considered a regression by my script). So it looks
> like
>       'fail' means either a 'FAILED' printout or a nonzero exit code. Which
>       would be fine; I just need to know.

see above.

> 
>   (b) On the other hand, it looks like 'skip' tests often print 'FAILED' but
>       return zero. In a shell testing environment I assumed that it would be

A test which terminates with exit code 0 shouldn't be marked as "skip". These maniftests were created using failure data from some time ago and will need to be cleaned up to reflect the current status of the tree. While I've run the browser version I've been unable to do the same with the shell. 

>       fine to crash, so I run all 'skip' tests and allow them to crash. This
> way
>       it is detected if they are fixed. But perhaps you really want 'skip'
> tests
>       not to run, even in shell?

I think we will be able to change the use of "skip" for the shell tests to only be used for obsolete tests or tests which do not terminate in a timely manner. This will allow us to not run the obsolete tests or the long running tests whil still running the "crashers". The current manifests don't reflect that choice though.

> 
> 2. My output format is for dev testing: you get a progress bar while it runs,
> and a summary report at the end. Do we want an output format that prints things
> like 'TEST-PASS' and 'TEST-UNEXPECTED-FAIL'?

Yes. From my point of view having the tests mirror the reporting used in the reftests and jsreftests so that the output can be processed on the tinderbox and pushlogs without modification is a big win.

> 
> 3. What should be done for tests listed in the manifest that are not found? And
> also for test files in the dirs that are not listed in the manifest?

If a listed test is not found, I would marked it as an TEST-UNEXPECTED-FAIL. 

Ignore directories not listed in the manifest. 

I'll take a closer look at the script on Saturday.
(Assignee)

Comment 5

9 years ago
Created attachment 399806 [details] [diff] [review]
Patch 4 -- uses manifest files

This patch incorporates most of bc's comments, I think. Most significantly it works based on manifest files to note known failures and skipped tests. In order to use it you will need a set of manifest files. I will post a reasonable set of manifest files separately. Some questions/comments for bc:

- How exactly are subtest results to be shown? They print out from the tests like this:

 PASSED! Section 1 of test - 11.5.1 - Equality Operators
 PASSED! Section 2 of test - 11.5.1 - Equality Operators
 PASSED! Section 3 of test - 11.5.1 - Equality Operators

Exactly what should the tinderbox-format output look like here?

- It might be useful to add a 'tags' feature to the manifest file format. The idea is to allow arbitrary tag strings to be associated with each test case, which could then be used for filtering or other controls in the harness. For example, I wanted tags 'slow' (to indicate a slow-running test) and 'perf' (to indicate a performance test). An 'obsolete' tag could also be useful.

- The expected return codes could also be controlled using the manifest.
(Assignee)

Comment 6

9 years ago
Created attachment 399810 [details] [diff] [review]
Manifest file collection

Run with this set using

    python3.1 jstests.py -m my.list <js>

Comment 7

9 years ago
(In reply to comment #5)

David, this is absolutely great! I owe you food/drink the next time I'm in MV.

> Created an attachment (id=399806) [details]
> Patch 4 -- uses manifest files
> 
> This patch incorporates most of bc's comments, I think. Most significantly it
> works based on manifest files to note known failures and skipped tests. In
> order to use it you will need a set of manifest files. I will post a reasonable
> set of manifest files separately. Some questions/comments for bc:
> 
> - How exactly are subtest results to be shown? They print out from the tests
> like this:
> 
>  PASSED! Section 1 of test - 11.5.1 - Equality Operators
>  PASSED! Section 2 of test - 11.5.1 - Equality Operators
>  PASSED! Section 3 of test - 11.5.1 - Equality Operators
> 

Tests which use reportCompare(), etc. record their subtest results in an array gTestcases. I append the js-test-driver-end.js script to the list of running scripts for a given test which simply calls jsTestDriverEnd() to clean up after the test and to dump each of the results from gTestcases. 

The current dump output in the shell version is geared towards the older Sisyphus-based log post processing which I don't want to propagate any further.

When the browser runs using the jsreftest.html loader, the new dump output for the browser defers to the reftest extension to perform the dumping of individual tests.

> Exactly what should the tinderbox-format output look like here?
> 

I think that keeping the shell and browser outputs as close to identical as possible is important. If they match the current reftest style output, I see no problem in reusing the existing tinderbox setups for the JavaScript tests. 

You can see example logs from opt and debug browser jsreftest runs here:

http://bclary.com/log/2009/09/10/jstestbrowser-opt.log
http://bclary.com/log/2009/09/10/jstestbrowser-debug.log

I can work up a followup patch to change the shell testcase dump to match the reftest style.

> - It might be useful to add a 'tags' feature to the manifest file format. The
> idea is to allow arbitrary tag strings to be associated with each test case,
> which could then be used for filtering or other controls in the harness. For
> example, I wanted tags 'slow' (to indicate a slow-running test) and 'perf' (to
> indicate a performance test). An 'obsolete' tag could also be useful.
> 
> - The expected return codes could also be controlled using the manifest.

I am constrained by my desire to have the JavaScript tests be handled identically for the shell and browser. By reusing the layout reftest extension to run the JavaScript tests in the browser we are constrained to that which is supported there or that which we can convince dbaron to support. We could fork reftests.js and create a js-only version for JavaScript testing, but I'm of the inclination that the existing reftest framework and manifest format and failure types get us more than 80% of the way to perfection.

With that said, if the content following the comment marker could be formalized in a way useful for the shell that would allow the format visible to reftest.js to remain the same.

In bug 469718, Jesse really pushed to not have separate manifest files for the shell and browser. I am working on consolidating them into a single format geared for the browser reftests, e.g. script ../../jsreftest.html?test=ecma_5/Date/15.9.4.2.js instead of script ecma_5/Date/15.9.4.2.js. The shell test runner would have to strip the leading crap from the file name, if that isn't too much trouble.

From your manifest collection, it appears you have everything working except the conditional processing of the failure types. I believe it will be possible to use a jstests.py.in that is preprocessed during the build to create a sandbox containing a subset of the values supported by the reftest sandbox. See http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest.js#260 I believe that capturing the conditional in the if clause and using Python's eval is doable.

Does your script require Python 3.1?

I'm working on the browser jsreftests today. Let me see how your work can be adapted to that.

Comment 8

9 years ago
David, the "print as a function" is Python 3.0 (or 2.6 with import from future). I think the standard environments are pegged at 2.5. Ted, is that right? 

I can rework the prints to use the old style print if you like. Are there other 3.x isms in the code you are aware of?
We currently require only Python 2.4 to build, but I believe all of our build slaves have 2.5 installed.

Comment 10

9 years ago
It appears the use of str.format() is also 3.0 fodder.
(Assignee)

Comment 11

9 years ago
I think those are the main 3.x-isms I used. There could be a library API change or two as well. I can do it, but I am busy with a topcrash now so I probably won't be able to get to it for a bit. I also need to update for your new manifest format.

Comment 12

9 years ago
Ok. thanks. I did a quick change of print function to print statement and removed the use of format in favor of the old style string formatting. I then ran into:

$ python jstests.py -m jstests.list
  File "jstests.py", line 169
    JS, *args = args
        ^
SyntaxError: invalid syntax

Not sure what this is. I'm fine with whatever works with your schedule. I was just trying to belatedly keep my promise of looking at it more. Let me know if there is anything I can do to help.
(Assignee)

Comment 13

9 years ago
Ok, people seem to want this so I'll try to get to it in the next week or so. I think the only big thing I need to do is figure out how to read and use the enhanced manifest file format. Landing will also require backporting to Python 2.5, but that should be easy enough.
We only currently require Python 2.4 to build Mozilla (but I think all our build machines have 2.5).
(Assignee)

Comment 15

9 years ago
Created attachment 408493 [details] [diff] [review]
Patch 5 (new manifest format and Python 2 backport)

Here's the new version, which supports the new manifest file format and runs in Python 2.5 (and hopefully 2.4). Questions:

- What do the 'random' and 'random-if' manifest file tags mean? I looked in the reftests README.txt and it says the output is not to be considered in seeing if the test fails. I'm not sure what that would mean in this context.

- As of now, the manifest files (jstests.list) in the tree don't match my shell results. Do they need updating? Here are the differences:

FIXES
    js1_6/extensions/regress-475144.js
    js1_7/extensions/regress-455982-01.js
    js1_7/extensions/regress-455982-02.js
    js1_8/extensions/regress-452476.js
    js1_8/extensions/regress-476427.js
    js1_8_1/trace/trace-test.js
REGRESSIONS
    e4x/decompilation/decompile-xml-escapes.js
    e4x/Expressions/11.1.4-08.js
    e4x/Global/13.1.2.1.js
    e4x/Namespace/regress-292863.js
    e4x/TypeConversion/10.2.1.js
    ecma/Date/15.9.5.11-2.js
    ecma/Date/15.9.5.12-2.js
    ecma/TypeConversion/9.3-1.js
    ecma_3/Number/15.7.4.6-1.js
    ecma_3/Number/15.7.4.7-1.js
    ecma_3/Object/8.6.2.6-002.js
    ecma_3/Operators/order-01.js
    ecma_3/String/15.5.4.11.js
    ecma_3/String/regress-392378.js
    ecma_3_1/RegExp/regress-305064.js
    js1_5/Exceptions/regress-333728.js
    js1_5/extensions/regress-322957.js
    js1_5/extensions/regress-435345-01.js
    js1_5/extensions/regress-50447-1.js
    js1_5/Regress/regress-313967-01.js
    js1_5/Regress/regress-3649-n.js
    js1_5/String/regress-314890.js
    js1_7/decompilation/regress-349634.js
    js1_7/extensions/regress-353214-02.js
    js1_8_1/extensions/strict-warning.js
    js1_8_1/String/regress-305064.js
    js1_8_1/trace/regress-452498-01.js
Attachment #395979 - Attachment is obsolete: true
Attachment #399806 - Attachment is obsolete: true
Attachment #399810 - Attachment is obsolete: true

Comment 16

9 years ago
(In reply to comment #15)

Thanks! I'll apply and try it out.

> - What do the 'random' and 'random-if' manifest file tags mean? I looked in the
> reftests README.txt and it says the output is not to be considered in seeing if
> the test fails. I'm not sure what that would mean in this context.

basically they are marked as REFTEST TEST-PASS(EXPECTED RANDOM) or REFTEST TEST-KNOWN-FAIL(EXPECTED RANDOM) so they don't show up as unexpected. Ideally, we should go back and either fix the test to be deterministic or disable/remove it altogether. The one benefit to leaving them in, is they can occasionally crash. :-)

> 
> - As of now, the manifest files (jstests.list) in the tree don't match my shell
> results. Do they need updating? Here are the differences:
> 
> FIXES

tests marked as failing that pass for you?

>     js1_6/extensions/regress-475144.js

$ grep 475144 js1_6/extensions/jstests.list 
fails-if(!xulRuntime.shell) script regress-475144.js # NS_ERROR_DOM_NOT_SUPPORTED_ERR

I've been using xulRuntime.shell as an indicator the test runs in the shell or not. If your driver can treat that as true, then this test would not have been marked as failing.

>     js1_7/extensions/regress-455982-01.js
>     js1_7/extensions/regress-455982-02.js
>     js1_8/extensions/regress-452476.js
>     js1_8/extensions/regress-476427.js
>     js1_8_1/trace/trace-test.js

ditto...

> REGRESSIONS

tests marked as passing that fail for you?

>     e4x/decompilation/decompile-xml-escapes.js

marked as failing.

>     e4x/Expressions/11.1.4-08.js

marked as failing.

>     e4x/Global/13.1.2.1.js

marked as failing.

>     e4x/Namespace/regress-292863.js

marked as failing.

>     e4x/TypeConversion/10.2.1.js

marked as failing.

>     ecma/Date/15.9.5.11-2.js

this one is marked as passing. I don't see failures for this test in my older sisyphus based tests.

>     ecma/Date/15.9.5.12-2.js

this is marked as crashing on browser 64 bit linux, but should pass in shell. I don't see failures for this test in my older sisyphus based tests.

>     ecma/TypeConversion/9.3-1.js

this one is marked as passing. I don't see failures for this test in my older sisyhphus based tests.

>     ecma_3/Number/15.7.4.6-1.js

this one is marked as passing. I *do* show this as failing in my sisyphus based js tests. I haven't seen this in my local runs of jsreftests in the browser though...

>     ecma_3/Number/15.7.4.7-1.js

marked as failing.

>     ecma_3/Object/8.6.2.6-002.js

marked as failing.

>     ecma_3/Operators/order-01.js

marked as failing.

>     ecma_3/String/15.5.4.11.js

marked as failing.

>     ecma_3/String/regress-392378.js

marked as failing.

>     ecma_3_1/RegExp/regress-305064.js

marked as failing.

>     js1_5/Exceptions/regress-333728.js

marked as failing.

>     js1_5/extensions/regress-322957.js

marked as failing.

>     js1_5/extensions/regress-435345-01.js

marked as failing.

>     js1_5/extensions/regress-50447-1.js

marked as passing. I don't show this failing in my older sisyphus based tests.

>     js1_5/Regress/regress-313967-01.js

marked as random. This is one of the BigO tests, that should be written to be more deterministic.

>     js1_5/Regress/regress-3649-n.js

marked as failing, hanging or no results depending on os/cpu. Does your code handle multiple conditions in the manifest?

>     js1_5/String/regress-314890.js

random BigO.

>     js1_7/decompilation/regress-349634.js

marked as failing.

>     js1_7/extensions/regress-353214-02.js

marked as failing.

>     js1_8_1/extensions/strict-warning.js

marked as failing for not following convention. Needs a call to reportCompare.

>     js1_8_1/String/regress-305064.js

marked as failing.

>     js1_8_1/trace/regress-452498-01.js

marked as failing.

Comment 17

9 years ago
danger will robinson! bug 471579 changed XPCOMABI to be a property on xulRuntime. I'm fixing the manifests that were broken in bug 524666
(Assignee)

Comment 18

9 years ago
Thanks. That helped a bunch. I had a few bugs in the manifest file processing, as well as a couple other boring errors, which were easy to find with your comments. Here are 2 that still don't match up:

- js1_5/Regress/regress-3649-n.js

The fails/random/skip notations only apply to Windows and Linux, so on my Mac it runs and is expected to pass. The test output is:

BUGNUMBER: 3649
STATUS: gc-checking branch callback.
--- NOTE: IN THIS TESTCASE, WE EXPECT EXIT CODE 0 ---
--- NOTE: IN THIS TESTCASE, WE EXPECT EXIT CODE 5 ---
./js1_5/Regress/regress-3649-n.js:59: InternalError: allocation size overflow

but the return code is 3, so it is marked as failing.

- ecma/TypeConversion/9.3-1.js

The output is:

begin test: ecma/TypeConversion/9.3-1.js
9.3-1 ToNumber

Previously, I think you told me that a test with no output is considered to fail. I interpreted that to mean that a test that prints no "PASSED!" output fails (unless a specific exception is in play, like the -n tests).
(Assignee)

Comment 19

9 years ago
Created attachment 408649 [details] [diff] [review]
Patch 6 (supports random, fix a bug or 2)

One thing I'm wondering about is how useful the 'random' tests are for developer testing. If they can succeed or fail randomly, then it would seem the results are going to be mostly ignored.
Attachment #408493 - Attachment is obsolete: true
They're more useful for actual reftests, where some graphical operations really can have unpredictable results. They're a little better than skip because the test still gets run, so we would still know if the test crashes.
(Assignee)

Comment 21

9 years ago
Created attachment 409464 [details] [diff] [review]
Patch 7 (treats crashes specially, option not to run random tests)

OK, I think it's review-worthy now. Basic usages:

  # run in dev test mode--run with -d to skip random tests
  python jstests.py <path-to-js>
  # run in tinderbox output mode
  python jstests.py <path-to-js> --tinderbox
  # show other options
  python jstests.py -h
Attachment #408649 - Attachment is obsolete: true
Attachment #409464 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #409464 - Flags: review?(jorendorff)
Attachment #409464 - Flags: review?(bclary)
Attachment #409464 - Flags: review?

Comment 22

9 years ago
Comment on attachment 409464 [details] [diff] [review]
Patch 7 (treats crashes specially, option not to run random tests)

It will take me some time (if ever) to be able to fully understand this code, but this review is based on my testing it. It might be useful if we met along with anyone else who is interested in this.

Personal Observation:

This may just be me, but having the js test output separated from the summary results makes it more difficult to review the results. Having the option to only output the summary results is great, but when I want to review the actual test output and compare to the summary results, I find it easier to have the summary results follow the test output in the same file. But as I am not the primary consumer, it is not a blocker for me.

Options:

How would you pass options to the js shell? Do you pass the full command line with arguments as a quoted string as the first argument of jstests.py?

Request for enhancement. jgriffin and I have been working on getting the unitests running under valgrind and purify on the unit test machines. He added --debugger and --debugger-args to the reftest and xpcshell test runners to match the mochitest test runner. It would be great if we had a similar functionality here. Although if the functionality is available via the command line as above, then I guess that is ok too.

--run-skipped is great however it treats an included skipped test as an expected pass, so if the test does pass the results are kind of mixed up with all of the other passing tests. It would be nice if a skipped test was treated as expecting to fail, then if it did pass it would appear as an UNEXPECTED-PASS so it would be easier to find the tests which should have the skip removed.

Summary Output:

The summary output to stdout does not identify the subtest being executed. For example, for js1_8_1/trace/math-trace-tests.js you see a number of identical lines:

TEST-PASS | ./js1_8_1/trace/math-trace-tests.js
TEST-PASS | ./js1_8_1/trace/math-trace-tests.js
TEST-PASS | ./js1_8_1/trace/math-trace-tests.js
....


In the browser jsreftest output you see

REFTEST TEST-PASS | file:///work/mozilla/builds/1.9.3-tracemonkey/mozilla/js/src/tests/jsreftest.html?test=js1_8_1/trace/math-trace-tests.js | Math.abs(void 0) Expected value 'NaN', Actual value 'NaN'  item 1
REFTEST TEST-PASS | file:///work/mozilla/builds/1.9.3-tracemonkey/mozilla/js/src/tests/jsreftest.html?test=js1_8_1/trace/math-trace-tests.js | Math.abs(null)  item 2
REFTEST TEST-PASS | file:///work/mozilla/builds/1.9.3-tracemonkey/mozilla/js/src/tests/jsreftest.html?test=js1_8_1/trace/math-trace-tests.js | Math.abs(true)  item 3

which is more informative and helpful I think. With the exception of tests which fail to terminate properly, we can make the js test output these lines using the same approach that is used in the jsreftest.html.

Testing results:

I run the tests with --run-skipped and the default 2 thread execution. Once I eliminated the obsolete tests (found in spidermonkey-n-1.9.3.tests) I reviewed the results and found several problems.


===

ecma/TypeConversion/9.3-1.js was an unexpected failure do its missing a call to test() which outputs the " PASSED!" and " FAILED!" lines. Normally I include js-test-driver-end.js following the test which masked this problem by outputting the summary results anyway.

===

ecma_3/Array/regress-322135-03.js
ecma_3/Array/regress-322135-04.js

They were marked as slow in the manifest.

Both showed rc=0, and an out of memory error.

But when I ran them manually the actual exit code was 5.

===

ecma_3/RegExp/regress-307456.js no pass/fail output.
ecma_3/RegExp/regress-330684.js no pass/fail output.

This was marked as slow in the manifest.

This showed as finishing very quickly in less than a second, but running it manually
it took much longer. I killed the manual tests, so I'm not sure of the exit code.

===

js1_5/Array/regress-465980-02.js

Marked slow in the manifest.

This showed rc=0, and an out of memory error and a run time of 0.08 seconds.

Running the test manually it returns exit code 5 and took 55 seconds on my macbook.

===

js1_5/GC/regress-346794.js

Marked as slow in the manifest.

This showed rc=0, with no pass/fail output and a run time of 8 seconds.

Running the test manually took much longer (I killed it).

===

js1_5/Regress/regress-3649-n.js

This test shows mucho funkiness across platforms.

This showed rc=0, with no pass/fail output, and an uncaught InternalError allocation size overflow.

When run manually, it returns exit code 3.

===

js1_5/extensions/regress-335700.js

BigO needs to die.


===


js1_5/extensions/regress-345967.js

This is marked slow in the manifest.

This showed rc=0, with no pass/fail output and a run time of 0.08 seconds.

When run manually, this test takes much longer for me (I killed it).

===

js1_5/extensions/regress-350531.js

This test is marked slow in the manifest.

This showed rc=0, with no pass/fail output and a run time of 1.5 seconds.

When run manually, this test takes much longer for me (I killed it).

===

js1_5/extensions/regress-50447-1.js


 FAILED! [reported from test2()] toSource() returned unexpected result. : Expected value '(new InternalError("msg", "./js1_5/extensions/regress-50447-1.js", 141))', Actual value '(new InternalError("msg", "././js1_5/extensions/regress-50447-1.js", 141))' 
 FAILED! [reported from test2()] fileName property returned unexpected value. : Expected value './js1_5/extensions/regress-50447-1.js', Actual value '././js1_5/extensions/regress-50447-1.js' 
 FAILED! [reported from test3()] toSource() returned unexpected result. : Expected value '(new InternalError("msg", "./js1_5/extensions/regress-50447-1.js", 10))', Actual value '(new InternalError("msg", "././js1_5/extensions/regress-50447-1.js", 10))' 
 FAILED! [reported from test3()] fileName property returned unexpected value. : Expected value './js1_5/extensions/regress-50447-1.js', Actual value '././js1_5/extensions/regress-50447-1.js' 

jstests.py has a redundant ./ at the beginning of the test paths.

===

js1_6/extensions/regress-456826.js

This test is marked slow in the manifest.

This test showed rc=0, with no pass/fail output, but an out of memory message.

When run manually terminated with a SIGBUS for me. bug 504632, or bug 505081.

===

js1_8/regress/regress-455981-01.js known bug 499524

===

js1_8_1/regress/regress-452498-168-2.js

This test's output was truncated by jstests.py.
Attachment #409464 - Flags: review?(bclary) → review-

Updated

9 years ago
Attachment #409464 - Attachment is patch: true
Attachment #409464 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 24

9 years ago
Created attachment 409808 [details] [diff] [review]
Patch 8 (feature reqs from bc)
Attachment #409464 - Attachment is obsolete: true
Attachment #409464 - Flags: review?(jorendorff)
(Assignee)

Comment 25

9 years ago
(In reply to comment #22)
> Personal Observation:
> 
> This may just be me, but having the js test output separated from the summary
> results makes it more difficult to review the results. 

Does the -o option not do what you want? Btw, I personally usually don't use the -o option in main runs, I just rerun on the failures individually or as a group to see what happened.

> Options:

I added all these in patch 8.

> Summary Output:

Also added in patch 8.

> Testing results:
> 
> I run the tests with --run-skipped and the default 2 thread execution. Once I
> eliminated the obsolete tests (found in spidermonkey-n-1.9.3.tests) I reviewed
> the results and found several problems.
> 
> 
> ===
> 
> ecma/TypeConversion/9.3-1.js was an unexpected failure do its missing a call to
> test() which outputs the " PASSED!" and " FAILED!" lines. Normally I include
> js-test-driver-end.js following the test which masked this problem by
> outputting the summary results anyway.

Can we just add the missing call? It seems more reliable to process the direct output rather than summarizing it at the end of the JS run (which won't run if there is an error or crash).

> ===
> 
> ecma_3/Array/regress-322135-03.js
> ecma_3/Array/regress-322135-04.js
> 
> They were marked as slow in the manifest.
> 
> Both showed rc=0, and an out of memory error.
> 
> But when I ran them manually the actual exit code was 5.

What platform are you on? Here is my output for these 2 tests, which I think is what we expect:

host-7-8:tests dmandelin$ python jstests.py ../debug/shell/js -so --tinderbox 322135-03 322135-04 --run-skipped
../debug/shell/js -f shell.js -f ecma_3/shell.js -f ecma_3/Array/shell.js -f ./ecma_3/Array/regress-322135-04.js -f js-test-driver-end.js
    rc = 5, run time = 32.799759
begin test: ecma_3/Array/regress-322135-04.js
BUGNUMBER: 322135
STATUS: Array.prototype.unshift on Array with length 2^32-1
STATUS: This bug passes if it does not cause an out of memory error
STATUS: Other issues related to array length are not tested.
./ecma_3/Array/regress-322135-04.js:57: out of memory
TEST-KNOWN-FAIL | ecma_3/Array/regress-322135-04.js
../debug/shell/js -f shell.js -f ecma_3/shell.js -f ecma_3/Array/shell.js -f ./ecma_3/Array/regress-322135-03.js -f js-test-driver-end.js
    rc = 5, run time = 32.988043
begin test: ecma_3/Array/regress-322135-03.js
BUGNUMBER: 322135
STATUS: Array.prototype.splice on Array with length 2^32-1
STATUS: This bug passes if it does not cause an out of memory error
STATUS: Other issues related to array length are not tested.
./ecma_3/Array/regress-322135-03.js:59: out of memory
TEST-KNOWN-FAIL | ecma_3/Array/regress-322135-03.js
PASS


> ===
> 
> ecma_3/RegExp/regress-307456.js no pass/fail output.
> ecma_3/RegExp/regress-330684.js no pass/fail output.
> 
> This was marked as slow in the manifest.
> 
> This showed as finishing very quickly in less than a second, but running it
> manually
> it took much longer. I killed the manual tests, so I'm not sure of the exit
> code.

I get:

../debug/shell/js -f shell.js -f ecma_3/shell.js -f ecma_3/RegExp/shell.js -f ./ecma_3/RegExp/regress-307456.js -f js-test-driver-end.js
    rc = -9, run time = 45.013717
begin test: ecma_3/RegExp/regress-307456.js
BUGNUMBER: 307456
STATUS: Do not Freeze with RegExp
STATUS: <!---<->---->
STATUS: 
STATUS: <><>--!!!<><><><><><>

[[snip a zillion status lines]]
STATUS: 
TEST-KNOWN-FAIL | ecma_3/RegExp/regress-307456.js
../debug/shell/js -f shell.js -f ecma_3/shell.js -f ecma_3/RegExp/shell.js -f ./ecma_3/RegExp/regress-330684.js -f js-test-driver-end.js
    rc = -9, run time = 45.014840
begin test: ecma_3/RegExp/regress-330684.js
BUGNUMBER: 330684
STATUS: Do not hang on RegExp
TEST-KNOWN-FAIL | ecma_3/RegExp/regress-330684.js
PASS


> ===
> 
> js1_5/Array/regress-465980-02.js
> 
> Marked slow in the manifest.
> 
> This showed rc=0, and an out of memory error and a run time of 0.08 seconds.
> 
> Running the test manually it returns exit code 5 and took 55 seconds on my
> macbook.

host-7-8:tests dmandelin$ python jstests.py ../debug/shell/js -so --tinderbox 465980-02.js --run-skipped
../debug/shell/js -f shell.js -f js1_5/shell.js -f js1_5/Array/shell.js -f ./js1_5/Array/regress-465980-02.js -f js-test-driver-end.js
    rc = 5, run time = 29.758304
begin test: js1_5/Array/regress-465980-02.js
BUGNUMBER: 465980
STATUS: Do not crash @ InitArrayElements
running testArrayPush(4294967294, [foo], false, 4294967295)...
running testArrayPush(4294967294, [foo, bar], true, 4294967295)...
running testArrayPush(4294967294, [foo, bar, baz], true, 4294967295)...
running testArrayPush(4294967295, [foo], true, 4294967295)...
running testArrayPush(4294967295, [foo, bar], true, 4294967295)...
running testArrayPush(4294967295, [foo, bar, baz], true, 4294967295)...
running testArrayUnshift(4294967294, [foo], false, 4294967295)...
./js1_5/Array/regress-465980-02.js:131: out of memory
TEST-KNOWN-FAIL | js1_5/Array/regress-465980-02.js
PASS
 
> ===
> 
> js1_5/GC/regress-346794.js
> 
> Marked as slow in the manifest.
> 
> This showed rc=0, with no pass/fail output and a run time of 8 seconds.
> 
> Running the test manually took much longer (I killed it).

I think this one (and possibly some of them above) could be from the timeout. By default, there is a 45s timeout (wall clock time) on each test. If you are running multiple workers or other processes, a given test might get not get scheduled to run that much at first, so it could die after less than 45 seconds of CPU time. I set 45 seconds because it seemed long enough to make the 'not skipped' tests run OK on my system. If you want to run slow tests then you probably want to use |-t 3600| or something like that.

host-7-8:tests dmandelin$ python jstests.py ../debug/shell/js -so --tinderbox 346794 --run-skipped
../debug/shell/js -f shell.js -f js1_5/shell.js -f js1_5/GC/shell.js -f ./js1_5/GC/regress-346794.js -f js-test-driver-end.js
    rc = -9, run time = 45.026191
begin test: js1_5/GC/regress-346794.js
BUGNUMBER: 346794
STATUS: Do not crash
--- NOTE: IN THIS TESTCASE, WE EXPECT EXIT CODE 0 ---
--- NOTE: IN THIS TESTCASE, WE EXPECT EXIT CODE 3 ---
TEST-KNOWN-FAIL | js1_5/GC/regress-346794.js
PASS


> ===
> 
> js1_5/Regress/regress-3649-n.js
> 
> This test shows mucho funkiness across platforms.
> 
> This showed rc=0, with no pass/fail output, and an uncaught InternalError
> allocation size overflow.
> 
> When run manually, it returns exit code 3.

I get the 3:

../debug/shell/js -f shell.js -f js1_5/shell.js -f js1_5/Regress/shell.js -f ./js1_5/Regress/regress-3649-n.js -f js-test-driver-end.js
    rc = 3, run time = 1.142932
begin test: js1_5/Regress/regress-3649-n.js
BUGNUMBER: 3649
STATUS: gc-checking branch callback.
--- NOTE: IN THIS TESTCASE, WE EXPECT EXIT CODE 0 ---
--- NOTE: IN THIS TESTCASE, WE EXPECT EXIT CODE 5 ---
./js1_5/Regress/regress-3649-n.js:59: InternalError: allocation size overflow
TEST-UNEXPECTED-FAIL | js1_5/Regress/regress-3649-n.js
REGRESSIONS
    js1_5/Regress/regress-3649-n.js
PASS

> ===
> 
> js1_5/extensions/regress-335700.js
> 
> BigO needs to die.

I agree with that. :-)
 
> ===
> 
> 
> js1_5/extensions/regress-345967.js
> 
> This is marked slow in the manifest.
> 
> This showed rc=0, with no pass/fail output and a run time of 0.08 seconds.
> 
> When run manually, this test takes much longer for me (I killed it).
> 
> ===
> 
> js1_5/extensions/regress-350531.js
> 
> This test is marked slow in the manifest.
> 
> This showed rc=0, with no pass/fail output and a run time of 1.5 seconds.
> 
> When run manually, this test takes much longer for me (I killed it).

This timed out for me at the 45s mark.

> ===
> 
> js1_5/extensions/regress-50447-1.js
> 
> 
>  FAILED! [reported from test2()] toSource() returned unexpected result. :
> Expected value '(new InternalError("msg",
> "./js1_5/extensions/regress-50447-1.js", 141))', Actual value '(new
> InternalError("msg", "././js1_5/extensions/regress-50447-1.js", 141))' 
>  FAILED! [reported from test2()] fileName property returned unexpected value. :
> Expected value './js1_5/extensions/regress-50447-1.js', Actual value
> '././js1_5/extensions/regress-50447-1.js' 
>  FAILED! [reported from test3()] toSource() returned unexpected result. :
> Expected value '(new InternalError("msg",
> "./js1_5/extensions/regress-50447-1.js", 10))', Actual value '(new
> InternalError("msg", "././js1_5/extensions/regress-50447-1.js", 10))' 
>  FAILED! [reported from test3()] fileName property returned unexpected value. :
> Expected value './js1_5/extensions/regress-50447-1.js', Actual value
> '././js1_5/extensions/regress-50447-1.js' 
> 
> jstests.py has a redundant ./ at the beginning of the test paths.

Interesting. I got a test failure on this test at first because jstests.py *wasn't* putting in an extra './'. But I only see the one './', not the two as in your output. I wonder what's going on here.

> ===
> 
> js1_6/extensions/regress-456826.js
> 
> This test is marked slow in the manifest.
> 
> This test showed rc=0, with no pass/fail output, but an out of memory message.
> 
> When run manually terminated with a SIGBUS for me. bug 504632, or bug 505081.

If slow, it's probably the timeout.
 
> ===
> 
> js1_8/regress/regress-455981-01.js known bug 499524

Is this not what you get and/or the wrong thing:

host-7-8:tests dmandelin$ python jstests.py ../debug/shell/js -so --tinderbox 455981-01 --run-skipped
../debug/shell/js -f shell.js -f js1_8/shell.js -f js1_8/regress/shell.js -f ./js1_8/regress/regress-455981-01.js -f js-test-driver-end.js
    rc = -6, run time = 6.327651
begin test: js1_8/regress/regress-455981-01.js
BUGNUMBER: 455981
STATUS: Do not assert: entry->localKind == JSLOCAL_ARG
Assertion failure: entry->localKind == JSLOCAL_ARG, at ../jsfun.cpp:2696
TEST-KNOWN-FAIL | js1_8/regress/regress-455981-01.js
PASS

> ===
> 
> js1_8_1/regress/regress-452498-168-2.js
> 
> This test's output was truncated by jstests.py.

It's a long-running test, probably a timeout issue.
This is a review of patch 7.

Please put spaces around the % operator.

It's good style to close files explicitly.

manifest.Parser could be written as a single function, parse(), returning a list.

I think manifest.Parser.parse_skip_if is dead.

manifest.ManifestFile{,Tree} look to be dead code.

tests.TestCase could be folded into tests.Test. (Nothing makes use of the fact that the base class is separate.)

tests.do_run_cmd could be rewritten in terms of test.th_run_cmd, saving some redundancy.

>+    return 'var xulRuntime = { OS: "%s", XPCOMABI: "%s", shell: true }; var isDebugBuild=%s;'%(
>+        self.abi,
>+        self.os,
>+        str(self.isdebug).lower())

abi and os are reversed here.

>+                    if val == '1':
>+                        b = True
>+                    else:
>+                        b = False
>+                    kw['isdebug'] = b

kw['isdebug'] = (val == '1')


In jstests.py:
>+ JS = None

This seems unnecessary.

>+    # Conceptually, this maps (test result x test expection) to text labels.
>+    #      key   is (result, expect, random)

(just a random thought) The table reveals that there are only 3 test expectations: pass, fail, and random. I'm ok with the table as-is though.

>+    if (OPTIONS.show_cmd or OPTIONS.show_output) and output_file == sys.stdout or OPTIONS.tinderbox:

Nit: Overparenthesize the `and`-expression here, like we do in C++?

>+        test_list = [ _ for _ in test_list if _.path in paths ]

Uber-nit: Using _ here isn't idiomatic in Python. I think it would be nicer to use a letter.

>+    if OPTIONS.random:
>+        test_list = [ _ for _ in test_list if not _.random ]

It looks like the sense of the flag is reversed. (?)

>+           # TODO Make an effort to print something when decode fails.

Yow, test output seems pretty important.

You can write: mybytes.decode(encoding, "replace") which will not throw an error; what we really want is more like "xmlcharrefreplace" than "replace", but I don't know if that was in Python 2.4.

>+    def __call__(self):
>+        if self.test.enable or OPTIONS.run_skipped:
>+            return self.test.run(self.js_path, OPTIONS.timeout)
>+        else:
>+            return NullTestOutput(self.test)

It seems like it would be better to filter out disabled tests before we get here. Then NullTestOutput could be dropped.

workers.Source.start seems to have some unused timing code in it.

Generally workers.py seems like a lot of code for what it does. Is all the logging and stuff really useful?

I'll look at patch 8 tomorrow--presumably not much has changed.

Comment 27

9 years ago
(In reply to comment #25)

> Does the -o option not do what you want? Btw, I personally usually don't use
> the -o option in main runs, I just rerun on the failures individually or as a
> group to see what happened.

Use the -o option and compare the js test output to the summary result collected in a different file. Don't you find that awkward ?

> 
> > Options:

I'll check them out.

> > 
> > ===

... most of your results match my manual results. I'll double check them.

> > 
> > ecma/TypeConversion/9.3-1.js was an unexpected failure do its missing a call to
> > test() which outputs the " PASSED!" and " FAILED!" lines. Normally I include
> > js-test-driver-end.js following the test which masked this problem by
> > outputting the summary results anyway.
> 
> Can we just add the missing call? It seems more reliable to process the direct
> output rather than summarizing it at the end of the JS run (which won't run if
> there is an error or crash).
> 

Yes. I just mentioned it here to explain that jstests.py was actually ok and the problem was with the test.

> > ===
> > 
> > ecma_3/Array/regress-322135-03.js
> > ecma_3/Array/regress-322135-04.js
> > 
> > They were marked as slow in the manifest.
> > 
> > Both showed rc=0, and an out of memory error.
> > 
> > But when I ran them manually the actual exit code was 5.
> 
> What platform are you on? Here is my output for these 2 tests, which I think is
> what we expect:

Older MacBook Pro, Dual Core, Mac OS X 10.5, Python 2.5.1 using the debug shell built with Firefox in $(objdir)/dist/bin/js

I ran with (iirc) a 300 second timeout to match the jsreftest default.

I did run with the latest patch in bug 522760.

Let me do another run with this latest patch. Your results are what I would expect. Not sure why mine differed.

> > ===
> > 
> > js1_8/regress/regress-455981-01.js known bug 499524
> 
> Is this not what you get and/or the wrong thing:
> 

I just mentioned this to note that jstests.py agreed with the manual approach.


> > ===
> > 
> > js1_8_1/regress/regress-452498-168-2.js
> > 
> > This test's output was truncated by jstests.py.
> 
> It's a long-running test, probably a timeout issue.

It didn't show the rc=... final line for me. Seemed like it didn't flush the buffer before terminating.

Let me try this version with a much longer timeout and get back to you.
Attachment #409808 - Attachment is patch: true
Attachment #409808 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 28

9 years ago
(In reply to comment #26)
> This is a review of patch 7.

All issues should be addressed in patch 9 except as noted below.

> It's good style to close files explicitly.

This seems pretty painful for files that are opened for reading, especially with the 'for line in open(foo):' idiom. Do you really think it's that important? In 2.6 it is easier to do it right with 'with', otherwise it seems not worth it.
 
> tests.TestCase could be folded into tests.Test. (Nothing makes use of the fact
> that the base class is separate.)

I know. I kept them separate for logical reasons. 'Test' represents a program to be run. 'TestCase' represents a test case, which includes a program to be run and results to be expected.
 
> >+    return 'var xulRuntime = { OS: "%s", XPCOMABI: "%s", shell: true }; var isDebugBuild=%s;'%(
> >+        self.abi,
> >+        self.os,
> >+        str(self.isdebug).lower())
> 
> abi and os are reversed here.

I did address this in patch 9 of course. I just wanted to say "Eeek!". And thanks.
 
> >+    # Conceptually, this maps (test result x test expection) to text labels.
> >+    #      key   is (result, expect, random)
> 
> (just a random thought) The table reveals that there are only 3 test
> expectations: pass, fail, and random. I'm ok with the table as-is though.

OK. They are separated for logical reasons again. And also because I went through many many iterations of figuring out how to represent the different test result classifications.
 
> >+        test_list = [ _ for _ in test_list if _.path in paths ]
> 
> Uber-nit: Using _ here isn't idiomatic in Python. I think it would be nicer to
> use a letter.

Hmmmm. This is my practice in Python 2.x because I've been burned by the scoping. The bound list variable stays defined outside the list and I've created bugs that way. I wouldn't mind using a special variable that won't collide if you can suggest something good. 'elem'?
 
> >+           # TODO Make an effort to print something when decode fails.
> 
> Yow, test output seems pretty important.
> 
> You can write: mybytes.decode(encoding, "replace") which will not throw an
> error; what we really want is more like "xmlcharrefreplace" than "replace", but
> I don't know if that was in Python 2.4.

Doesn't seem to be. But the 'decode' calls are unnecessary, it turns out, they were only needed for Python 3.x.
 
> >+    def __call__(self):
> >+        if self.test.enable or OPTIONS.run_skipped:
> >+            return self.test.run(self.js_path, OPTIONS.timeout)
> >+        else:
> >+            return NullTestOutput(self.test)
> 
> It seems like it would be better to filter out disabled tests before we get
> here. Then NullTestOutput could be dropped.

I wasn't sure about this. That makes sense to me, too, but some messages from Bob seemed to imply the customary Tinderbox output included lines for skipped tests, so I did it this way.

> workers.Source.start seems to have some unused timing code in it.
> 
> Generally workers.py seems like a lot of code for what it does. Is all the
> logging and stuff really useful?

That's for debugging. Should it be stripped?
(Assignee)

Comment 29

9 years ago
Created attachment 411334 [details] [diff] [review]
Patch 9 (addresses some of jorendorff's review comments)
Attachment #409808 - Attachment is obsolete: true
I'm happy with all David's responses to my comments. Can't wait to see this land.
(Assignee)

Comment 31

9 years ago
Created attachment 411764 [details] [diff] [review]
Patch 10 (addresses further issues from discussion)

This takes care of the stuff we talked about yesterday. There is one change: instead of --debugger, --debugger-args, and --debugger-interactive arguments, I made --valgrind and --debug (or -g for short). I just found this easier to make sense out of, since the other system requires a lot of coordination to make the options consistent.

There is also a new option --check-manifest (or -c), that looks for JS tests that are not in any manifests. I did find one test that way, regress-522123, which I added to the manifest.
Attachment #411334 - Attachment is obsolete: true
Attachment #411764 - Flags: review?

Comment 32

9 years ago
Created attachment 412341 [details] [diff] [review]
manifest changes patch

I ran jstests.py and like what I see so far. This is my first stab at updating the manifests after running with --run-only-skipped. I ran into a problem with one of the updated manifest entries though:

js1_7/extensions/jstests.list:skip-if(!xulRuntime.shell||xulRuntime.shell&&xulRuntime.XPCOMABI.match(/x86_64/)) script regress-458679.js # slow

Traceback (most recent call last):
  File "jstests.py", line 256, in <module>
    test_list = manifest.parse(OPTIONS.manifest, xul_tester)
  File "/work/mozilla/builds/1.9.3-tracemonkey/mozilla/js/src/tests/manifest.py", line 97, in parse
    ans += parse(os.path.join(dir, parts[1]), xul_tester)
  File "/work/mozilla/builds/1.9.3-tracemonkey/mozilla/js/src/tests/manifest.py", line 97, in parse
    ans += parse(os.path.join(dir, parts[1]), xul_tester)
  File "/work/mozilla/builds/1.9.3-tracemonkey/mozilla/js/src/tests/manifest.py", line 125, in parse
    if xul_tester.test(cond):
  File "/work/mozilla/builds/1.9.3-tracemonkey/mozilla/js/src/tests/manifest.py", line 78, in test
    raise Exception("Failed test test XUL condition '%s'"%cond)
Exception: Failed test test XUL condition '!xulRuntime.shell||xulRuntime.shell&&xulRuntime.XPCOMABI.match(/x86_64/)'

Also, should -c require the shell be specified?

I'll continue to look at some of the other features this weekend after I get back home.

Comment 33

9 years ago
PS. the browser reftests handled that manifest conditional ok.
(Assignee)

Comment 34

9 years ago
Created attachment 412349 [details] [diff] [review]
Patch 11 (-c without shell and parse new manifest entry)

This makes -c not need arguments. Also, I got that new manifest entry working. The issue was that the conditional returns 'null', and I was only looking for 'true' or 'false'.

By the way, I folded the manifest changes patch into this one but I can give it to you separately too if you want.
Attachment #411764 - Attachment is obsolete: true
Attachment #412349 - Flags: review?
Attachment #411764 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #412349 - Flags: review? → review?(bclary)

Comment 35

9 years ago
great, I'll give it a shot. If I have additional manifest changes, I'll just supply them as an additional patch that you can fold in as well.

Comment 36

9 years ago
Created attachment 414073 [details]
additional manifest changes

Comment 37

9 years ago
Comment on attachment 412349 [details] [diff] [review]
Patch 11 (-c without shell and parse new manifest entry)

Looks ok with 2 exceptions which I don't really care too much about.

1) requires to be run from js/src/tests. it does not take the directory of the specified manifest file and prepend that to the tests referenced in the manifests.

2) when the manifest is specified with a leading ./, it does not strip the leading ././ from the test file paths it loads leading to some failures that expect the paths to be of the form ./...

python jstests.py ../Darwin_OPT.OBJ/js  -so --tinderbox -m ./jstests.list > out.txt

$ grep UNEXPECTED out.txt
TEST-UNEXPECTED-FAIL | ./js1_5/extensions/regress-50447-1.js | [reported from test2()] toSource() returned unexpected result. : Expected value '(new InternalError("msg", "./js1_5/extensions/regress-50447-1.js", 141))', Actual value '(new InternalError("msg", "././js1_5/extensions/regress-50447-1.js", 141))' 

I can live with either issue if others can, or we can fix in a later patch.

Lets get it in and get everyone using it and put jsDriver.pl on notice that its days are numbered.
Attachment #412349 - Flags: review?(bclary) → review+
(Assignee)

Comment 38

9 years ago
http://hg.mozilla.org/tracemonkey/rev/4b1f9afcedb1

Bob, I'd like to fix both of the issues you mention in comment 37. But I don't think I can get to it immediately, and I'd rather land. I've noted them but feel free to file followup bugs.
Whiteboard: fixed-in-tracemonkey

Updated

9 years ago
Blocks: 530774
(Assignee)

Updated

9 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.