Normalize testcase format

[bterlson]

This commit normalizes the test case format used across test262. It applies the following transformations:

• Convert to YAML for frontmatter
• Remove of trailing whitespace
• Replace /r/n with /n except in chapters 6 and 7.
• Copyright header always uses // comments
• new includes attribute replaces $INCLUDE
• No implicit assumptions about test environment other than $ERROR. Everything else appears in the include array. This includes "runTestCase" which is now included in a substantial number of tests.

[bterlson]

Ping @smikes, can you test with your recent harness modifications?

[bterlson]

I don't believe the web harness will support this so we will have to do work to make the website work. I think I want to pull this in before then, however, and work on updating the web harness once kinks with this normalization are worked out.

Also I've tested with an MSFT internal harness and this seems to work for us. Check it and see.

Lastly, a normalization I wanted to test was ensuring that negative always describes the error type it expects, but unfortunately there are many many tests that specify negative but don't specify an error, and these tests throw at least both Ref and Syntax errors in large numbers. This normalization would definitely be good, but for now we can just deprecate the negative flag and say people must use negative attribute (which requires specifying the error type)

[rwaldron]

I've reviewed all 11,825 files and LGTM.

;)

[smikes]

I will run a full "pre" and "post" this evening. My platform is node 0.11 / Windows 7. Can we find someone with a fast linux/mac machine to give a second opinion, possibly using js24 or similar?

[NorbertLindenberg]

• Unlike rwaldron, I have not reviewed all 11,825 files 😞
• What's the motivation for switching to YAML? Nothing against YAML, I just don't see what it enables that wasn't possible before.
• If you touch all copyright notices, you should also update the year to 2014 and the license URL to the current one on github.
• Many test cases have an @path attribute that's not needed. Are you getting rid of it?
• I'd prefer that the web harness work with these changes before they are pulled in – after all, the main purpose of test262 is to test browsers.

[bterlson]

What's the motivation for switching to YAML?

YAML doesn't enable anything, but it makes parsing test262 test cases simpler. The @-syntax is ad-hoc and fairly annoying to parse. Harnesses and other tools can (hopefully) import a YAML library to parse the frontmatter. It's also fairly human readable and writable.

If you touch all copyright notices, you should also update the year to 2014 and the license URL to the current one on github.

I had originally worked to normalize the Copyright headers but it turned out that there are enough minor variations of copyright message that it's not particularly trivial. Can probably spend some time with regexps to solve this but I think it's a problem better left for later.

Many test cases have an @path attribute that's not needed. Are you getting rid of it?

Sorry, yes, I should have mentioned that this includes removal of @path. I also want to include the @es5id attribute pulled from @path. I'll update this PR later with that included. I'll also look into what it would take to support this on the website.

[smikes]

Still verifying.

As a separate PR, will you publish the code that canonicalizes a test case? I believe this would be useful to test case authors, either as a linter or as a preformatter.

[anba]

Can we find someone with a fast linux/mac machine to give a second opinion, possibly using js24 or similar?

I can perform some tests runs with SpiderMonkey, JavaScriptCore and V8 (fairly recent builds, ~1w old; can rebuild from current source if necessary). Just need to know what exactly needs to be tested.

[smikes]

runTestCase.js is not committed.

Failed to convert YAML in ch15/15.2/15.2.3/15.2.3/15.2.3.1/S15.2.3.1_A3.js -- output is

/*---
---*/

/**
* @name: S15.2.3.1_A3;
* @section: 15.2.3.1, 15.2.4;
* @assertion: The Object.prototype property has the attribute DontDelete;
* @description: Checking if deleting "Object.prototype" property fails;
* @noStrict
*/

[smikes]

Some other helpers are not committed:

File	# times used
$FAIL.js	421
dataPropertyAttributesAreCorrect.js	304
fnGlobalObject.js	187
accessorPropertyAttributesAreCorrect.js	187
testIntl.js	75
$PRINT.js	25
Test262Error.js	25
Date_constants.js	10
Date_library.js	6
compareArray.js	3
arrayContains.js	1
fnExists.js	1

[updated to remove testBuiltInObject from list of missing helpers]

[smikes]

When I do the naive thing and copy all the named functions out into separate files, I start seeing errors with multiple function definition on cases that run under 'use strict':

Example:

ch07\7.8\7.8.4\7.8.4-9-s in strict mode:
--- errors ---
c:\users\sam\appdata\local\temp\test262-x0jbmn.js:935
}
^
SyntaxError: Variable 'runTestCase' has already been declared
    at exports.runInThisContext (vm.js:69:16)
    at Module._compile (module.js:432:25)
    at Object.Module._extensions..js (module.js:467:10)
    at Module.load (module.js:349:32)
    at Function.Module._load (module.js:305:12)
    at Function.Module.runMain (module.js:490:10)
    at startup (node.js:124:16)
    at node.js:807:3

Refactoring.

[smikes]

Completed test run.

This is amazing work Brian.

I have only one test difference between the pre-51 and post-51 runs. [RESOLVED - was running with wrong version of node, ES6 allows function declaration in for .. in { } block ]

@anba -- I had to add some harness files and make changes to the test262.py runner to make this work. I will commit these changes to my version of the test262 repo and then ask you to run some verifications as well.

[smikes]

OK. I have pushed my changes to github.com:smikes/test232.git on branch smikes:normalize-format-withyaml.

Suggested instructions for how to run a before/after comparison are in this gist: https://gist.github.com/smikes/81f473cd6ca800cbbdf9

We expect to observe the same successes and failures with both the pre-51 (master) and post-51 (normalize-format) versions of the tests.

[smikes]

Good news re: website support.

With one additional change (make common.py use the new YAML-aware parsing code in parseTestRecord.py) this can publish a working website. I will commit and push the relevant change to my branch.

I tested by running all of ch07 in Chrome, hosting the website locally, and it succeeded. Now running all tests in Chrome and I will compare it against the pre-51 version of the tests+website.

[anba]

We expect to observe the same successes and failures with both the pre-51 (master) and post-51 (normalize-format) versions of the tests.

Tests are now finished, no regressions in SpiderMonkey, JavaScriptCore and V8. 👍

[smikes]

Completed post-51 Chrome test of website. No regressions.

[smikes]

To summarize:

1. no regressions found - Tested on Windows [ node 0.11, Chrome 35 ]; OSx (?) [ SpiderMonkey, JavaScriptCore, V8 ]
2. works with both console and website runner (when patches from normalize-format-withyaml applied)

[bterlson]

Sorry @smikes, I should have mentioned that part of the normalization. I have tried so many different things in the last month I forgot what I kept and what I tossed. The part you noticed about the implicit includes is that I broke them up into smaller pieces. What you did (copy the functions out into separate files) is exactly what I have done so I think we can go with that. The benefit of having smaller pieces is that repro files are easier to deal with.

I also allow for these files to have frontmatter that include other library files, for example fnSupportsArrayIndexGettersOnObjects uses fnExists, but fnExists is also used directly by some tests. So I have fnExists.js and fnSupportsArrayIndexGettersOnObjects.js has a frontmatter include for fnExists.js. Not sure what tests these are required for off-hand but I'm surprised it just worked with your change!

I'll check out that weird test case you pointed above and see about fixing that as well.

@smikes, know how to send a PR to a PR? Might be nice to include your normalize-format-withyaml branch in this PR since it includes the necessary harness changes. I could do that manually I suppose. Let me know when you think it's ready to include along with this PR!

[smikes]

Don't worry about the one failing test. It was an artefact caused by having a different node version in one of my test windows, so the test showed an apparent failure in the one case that ES6 mattered -- allowing functions to be declared inside a block in strict mode.

[smikes]

I created a PR for the PR -- I think -- bterlson#1 -- let me know if I should change anything.

It turns out fnSupportsArrayIndexGettersOnObjects is never used. I removed it, and a few others, that I could not find with grep <name> -r test/suite

[smikes]

Re the recursive frontmatter - there were very few instances of actual recursion, and I solved those manually. So for example 'Test262Error.js' is empty except for comments; since the definitions of Test262Error and testFailed are needed for the basic form of the $ERROR function, I just included them in sta.js.

I added a --list-includes option to test262.py to collect information about which files are actually included; we can use that (later) to cull the test/harness directory.

I believe included harness files should not be allowed to include other harness files. If a test requires multiple files included then it should list them all explicitly. This will increase transparency and encourage test authors to use fewer include files.

Also, it would be difficult to support recursive include parsing with the current packager/web runner combo. The packager only reads the test cases, and not the includes. The website runner expects to add the includes without parsing them. Neither is well-set-up to take the responsibility for recursively resolving includes.

[bterlson]

@smikes awesome work! I was mostly joking asking for a PR for the PR but now that I see how it works... that's pretty cool.

I believe included harness files should not be allowed to include other harness files. If a test requires multiple files include d then it should list them all explicitly. This will increase transparency and encourage test authors to use fewer include files.

Also, it would be difficult to support recursive include parsing with the current packager/web runner combo. The packager only reads the test cases, and not the includes. The website runner expects to add the includes without parsing them. Neither is well-set-up to take the responsibility for recursively resolving includes.

Hmm, somewhat disappointing I guess. For broadly used items like fnExists it seems like it might be nice. But I suppose anything super common like fnExists can move to sta.js and helpers can only rely on sta.js functions (which are hopefully kept to as few as possible).

At this point it seems like we've validated that the collateral works, have updates to the harness and website. Remaining work is the following:

1. Add es5id attribute from file name.
2. Investigate weird output for S15.2.3.1_A3.js.

Anything else?

@smikes RE the code I'm using to do the normalization, I'm happy to share it if you would like to use it, though it's disgusting and I don't think belongs in test262 proper. I can put it up in a separate repo for you if you would find it useful.

[smikes]

Agree with your two points. 

Agree with the separate repo. Ugly is fine, working is the main thing!  Making a useful linter/formatter is a worthwhile target. 

Wrt es5id, is this something that needs to be in each test file? Or can the packager synthesize it from the file name at packaging time?

[edited to fix formatting and remove quoted text]

[bterlson]

I'm not sure it's useful as a linter/formatter but I'll put it up and see what you think. Much of the code is parsing the existing format which I've normalized so that task is much easier now :)

es5id has to be in each test file I think. It can't be synthesized because we will be moving test files to ES6 locations, effectively destroying that information. Generally speaking it seems more maintainable to specify in metadata what spec line the test is testing rather than in the file name because it lets us organize test files in more sane ways (eg. perhaps by feature name, or something).

[rossberg]

I'm generally fine with cleaning this up, but removal of the @Negative attribute broke our internal buildbot runner framework, which looked for this string to infer test expectations. The @-string was rather unambiguous, whereas the new replacement (a) diverges into at least two different forms ("negative: ..." and "Flags: [...negative...]"), each of which is (b) more difficult to grep for, and (c) less reliably distinguished from actual JavaScript content of the tests. Would it be possible to go for a more uniform and unambiguous format?

[smikes]

@rossberg-chromium Is there a repo for this runner framework? I could take a look at it and offer a PR to support the YAML-based frontmatter.

[bterlson]

@rossberg-chromium Thanks for taking a look! I agree that @-strings are easier to parse in that for flags you can just have 'indexOf' and be done, but other flags were difficult to parse (especially those that spanned multiple lines). I'm not sure how more uniform and unambiguous you can get using a format that has multiple interoperable parsers available and a fairly reasonable spec.

Having two ways to specify negative is something that I dislike, though. I explained above, but in a nutshell the problem is that not all tests specify what kind of error they are expecting and these under-specified tests throw multiple kinds of errors so I can't just give them some useful default value. My thinking was to for now have both a flag form and a associated list form, but if it would help, I can move all negative flags to the list form with a value like '.' for cases that don't otherwise specify. Then if you don't want to depend on YAML you could probably just use a regexp like /^negative: (.)$/m fairly safely.

Thoughts?

[bterlson]

Rather than add a new helper, I changed the tests to be negative: Test262Error which ensures it's a non-early error so should be fine. Then I do throw new Test262Error().

[bterlson]

Rebased. Ready to merge!!

[bterlson]

@rossberg-chromium I pulled this in, but if there are steps we can take to make this work better with your harness we can discuss it!

[rossberg]

Apologies for my silence, still catching up wit email after vacation.

We'll look into ways of dealing with the change.

@smikes, No the repo isn't public, unfortunately.

[smikes]

@rossberg-chromium after #67 goes in, you should be able to replace a check for '@Negative' with a check for 'negative:'

[domenic]

@smikes, it's https://code.google.com/p/v8/source/browse/branches/bleeding_edge/test/test262/testcfg.py by the way. Your help would be greatly appreciated in getting it up and running... Python is not my area of expertise.

[smikes]

Heh. Mine either. But for now you can handle the negative issue by modifying line 85

    return "@negative" in self.GetSourceForTest(testcase)

to

    source = self.GetSourceForTest(testcase)
    return ("@negative" in source) or ("negative:" in source)

There is a more-or-less general purpose test262 parser in test262 tools/packaging/parseTestRecord.py ; I will see if I can adapt it to v8's harness -- I have a v8 checkout, and I can at least send you a diff if not a pull request. (How 90s can I get?)

[domenic]

Ya, the V8 runner also seems to not be doing includes correctly, instead just always including the same 3 files?

[smikes]

That's reasonable, if it started life as a port of the test262 console runner. That one did not handle includes until recently, just including testIntl and a few others statically.

[smikes]

@domenic how free can I be in fiddling with that testcfg.py? Can I assume that data is a valid github checkout of test262 (or a symlink to same)?

[domenic]

@smikes from what I understand it needs to conform to the common interface that the other testcfg.py's have, i.e. have ListTests and GetFlagsForTestCase, IsNegativeTest, and other stuff in testsuite.py. And yeah, data will indeed be a valid checkout of test262.

[smikes]

@domenic I created a new issue over on google code / V8 to make notes on this, and to eventually attach the patch(es).

https://code.google.com/p/v8/issues/detail?id=3513

[smikes]

@domenic The patch has now landed in chromium / bleeding-edge ; they're keeping the old test262 as a frozen reference to the ES5 standard as of April 29, 2013. (changes since then: fbba29f...master )

The new test directory is test/test262-es6 with a reference to a recent test262 version ( hash 595a36b 2014/08/10 revision ) Someone (me?) should take a look at this every half year or so, updating this reference so that the Chromium project is getting the latest test262 tests.

[domenic]

@smikes you are an amazing human being <3. I think those of us implementing ES6+ features in V8 can be responsible for updating it in V8. For example I will probably submit my Array.prototype.contains tests to test262, then update the reference so that it includes those, and use the test262 tests to test my implementation of Array.prototype.contains in V8.

[smikes]

Since the test262-es6 fork is for the ES6 features, you will probably want
to supply --harmony to all the invocations of d8 and change the
test262-es6.status file to reflect that es6 tests are expected to pass (I
marked them all as [FAIL]). That's good because it neatly fixes the issue
I mentioned in https://code.google.com/p/v8/issues/detail?id=3521 , but
without extending the testrunner context object.

On Fri, Aug 22, 2014 at 8:59 AM, Domenic Denicola notifications@github.com
wrote:

@smikes https://github.com/smikes you are an amazing human being <3

Actually I am a robot ;-)

[rossberg]

On 22 August 2014 18:13, smikes notifications@github.com wrote:

Since the test262-es6 fork is for the ES6 features, you will probably want
to supply --harmony to all the invocations of d8 and change the
test262-es6.status file to reflect that es6 tests are expected to pass (I
marked them all as [FAIL]). That's good because it neatly fixes the issue
I mentioned in https://code.google.com/p/v8/issues/detail?id=3521 , but
without extending the testrunner context object.

Good point, thanks! We had activated on our runners, but not in the cfg
itself. Done:

https://codereview.chromium.org/474423003/

/Andreas

[rossberg]

We noticed that test262 now requires YAML for Python. That causes a
bit of headache for us, since installing that dependency on all of our
distributed testing bot infrastructure is not a trivial task. We can
probably pull it off in a couple of days, by creating a git mirror of
the relevant YAML lib, but I wonder if there is a way to avoid this
additional dependency?

/Andreas

On 25 August 2014 16:34, Andreas Rossberg rossberg@google.com wrote:

On 22 August 2014 18:13, smikes notifications@github.com wrote:

Since the test262-es6 fork is for the ES6 features, you will probably want
to supply --harmony to all the invocations of d8 and change the
test262-es6.status file to reflect that es6 tests are expected to pass (I
marked them all as [FAIL]). That's good because it neatly fixes the issue
I mentioned in https://code.google.com/p/v8/issues/detail?id=3521 , but
without extending the testrunner context object.

Good point, thanks! We had activated on our runners, but not in the cfg
itself. Done:

https://codereview.chromium.org/474423003/

/Andreas

[bterlson]

@rossberg-chromium I was hoping the dep wouldn't be a big issue... There are ways to avoid it, sure, but I'm not sure how palatable any of them are...

1. Move to JSON instead of YAML? Still needs a JSON parser in Python though...
2. Build a parser for just the subset of YAML that we use and include it? I'm not sure how complex it would be but it could be easy...
3. If installing node dependencies is easier than python dependencies you could look at using the node harness.

Let me know if you have any thoughts...

[smikes]

I can try to write a native python YAML subset parser to use as a fall back if the YAML lib is not present. It will be fragile in that I will only target YAML we are using right now in the front matter.

On Wed, Aug 27, 2014 at 11:28 AM, Brian Terlson notifications@github.com
wrote:

@rossberg-chromium I was hoping the dep wouldn't be a big issue... There are ways to avoid it, sure, but I'm not sure how palatable any of them are...

1. Move to JSON instead of YAML? Still needs a JSON parser in Python though...
2. Build a parser for just the subset of YAML that we use and include it? I'm not sure how complex it would be but it could be easy...
3. If installing node dependencies is easier than python dependencies you could look at using the node harness.

Let me know if you have any thoughts...

Reply to this email directly or view it on GitHub:
#51 (comment)

[smikes]

I can try to write a native python YAML subset parser to use as a fall back if the YAML lib is not present. It will be fragile in that I will only target YAML we are using right now in the front matter.

Implemented and under test right now - running through the whole test suite to see if there is any frontmatter that breaks the "parser".

Handled:

• string, ints, double scalars
• Lists in both flow format [foo, bar] and multiline format - foo\n - bar
• one kind of multiline string (> flow continuation)

Not handled:

• anything that's not implicitly a map
• other kinds of multiline strings (| or + or explicit indent)
• anchors or tags
• the %YAML 1.2 syntax

Using a real YAML parser is still ideal, but this will make the tests runnable with just python.

[smikes]

I created a PR for the fallback parser (#90).

The changes reside at https://github.com/smikes/test262/tree/monkeyYaml

Assuming your test/test262-es6/data directory is a git checkout,

cd test/test262-es6/data
git add remote smikes https://github.com/smikes/test262
git checkout smikes/monkeyYaml

ought to give you a local checkout that you can test on a non-pyYAML machine.

[rossberg]

On 28 August 2014 13:57, smikes notifications@github.com wrote:

I created a PR for the fallback parser (#90
#90).

The changes reside at https://github.com/smikes/test262/tree/monkeyYaml

Assuming your test/test262-es6/data directory is a git checkout,

cd test/test262-es6/data
git add remote smikes https://github.com/smikes/test262
git checkout smikes/monkeyYaml

ought to give you a local checkout that you can test on a non-pyYAML
machine.

Cool, thanks for that, that's great! The patch seems to work, at least
locally. (We will be able to try it on the bot infrastructure once the
patch has landed.)

/Andreas