636175 - Move all JSON tests out of dom/src/json/test/unit

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

It's not part of the shell tests, so it's a nasty surprise when you tweak the JSON code, see the JS test suite pass, then land only to find tests off the beaten path start failing.  We'd have picked up bug 557371 for 4.0 if it weren't for those tests failing in exactly that manner (the change itself was correct, the dom/src/json tests themselves just weren't ready for that change).

One or two of these tests exercise code solely in dom/src/json.  That exercise needs to happen, but those tests can probably be vastly simplified, or can be made to only test things the JS suite tests.  I'm not certain what I'm going to do about those yet.  But most to all of them can be moved to js/src/tests without significant change.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Move tests, sometimes split them up, leave a very few in dom/src/json/test/unit if they actually test nsJSON.cpp

It might not be a bad idea to rubber-stamp this test-move, but more in-depth review also couldn't hurt.  Hopefully the comments at the top of the new tests pointing to their predecessors will help if you decide more attention is needed.

Comment 2

[Paul Biggar]

Comment on attachment 516074 [details] [diff] [review]
Move tests, sometimes split them up, leave a very few in dom/src/json/test/unit if they actually test nsJSON.cpp

> if (typeof reportCompare === "function")
>  reportCompare(true, true);

I don't understand this idiom. Is it still used in the jstests framework or is it a remnant of where the tests were before?

Was there a reason you rewrote some tests, and just copied others verbatim?

Where is fail13 gone?

The tests lack public domain copyright stuff.

I wonder if the "Ported" lines will still be useful after review - if not probably best to remove them before pushing.

I could be wrong, but stringify.js looks a lot shorter than test_wrappers.js. Did you move the rest of it elsewhere?

r+ with these addressed.

Comment 3

[Jeff Walden [:Waldo]]

(In reply to comment #2)
> > if (typeof reportCompare === "function")
> >  reportCompare(true, true);
> 
> I don't understand this idiom. Is it still used in the jstests framework or is
> it a remnant of where the tests were before?

The test harness expects reportCompare to be called at least once, to detect that the test actually tested something.  It's necessary for tests that just do their testing with the fail-immediately assertEq method.  I tend to prefer assertEq for that reason (less test failure output to read when debugging), but it does mean you have this gunk (typeof-guarded so the test can be run as a standalone file if written appropriately) at the end.

> Was there a reason you rewrote some tests, and just copied others verbatim?

Mostly readability for my own sanity as I expect to end up rereading parts of these while working on future patches.  Beyond that, there were redundancies obscuring things, making it harder to check whether some particular pattern is or isn't tested.  Also, some cleanup was motivated by getting rid of json2.js testing, because json2.js has the large disadvantage of munging toJSON methods onto standard prototypes, which substantially interferes with the spec algorithms (and the tests themselves, if left to stand).  

> Where is fail13 gone?

fail13.json is parse-octal-syntax-error.js now.  It's split out because we parse octal numbers right now even tho we shouldn't per spec -- the test is the only "fails" test in ecma_5/JSON.  Perhaps we can fix that soon, maybe this release if I finish the parser rewrite quickly enough.

> The tests lack public domain copyright stuff.

I didn't add PD to tests that didn't have it previously, to be license-cautious.

> I wonder if the "Ported" lines will still be useful after review - if not
> probably best to remove them before pushing.

It seems like a nice breadcrumb trail to leave for someone trying to do test archaeology.  It was useful when writing the patch, and I'm not certain enough no one will ever want to do such archaeology to be comfortable removing them.

> I could be wrong, but stringify.js looks a lot shorter than test_wrappers.js.

test_wrappers.js had a section that tested parsing, but that section duplicated a testing section at the end of test_decode.js -- a section now tested in parse.js.  So I think this bit is okay.

Comment 4

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/0756cd76cb06

Comment 5

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/0756cd76cb06