Add testcases for tamarin's native JSON implmentation.

An suggestion before I forget it: in Emacs, do: M-x flyspell-mode

dschaffe: Please review these suggested changes to your patch. If they are good, then fold them into attachment 526832 [details] [diff] [review] and post the folded result. (Sorry for the runaround about the error messages; I did not know that there was a standard routine for reducing them down to just their error code. I'll trust that you will decide on the actual right way to do these things.)

With the changes I suggested in attachment 526885 [details] [diff] [review], I currently am at this point with my development avmshell: FAILURES: ecma3/JSON/as3types.abc : Vectors of Boolean are stringified to JSON Array syntax = {"length":4,"fixed":false,"0":true,"1":false,"2":true,"3":false} FAILED! expected: [true,false,true,false] ecma3/JSON/invalid.abc : Invalid JSON unicode '\u00bb' = no exception FAILED! expected: SyntaxError: Error #1132: Invalid JSON input. ecma3/JSON/invalid.abc : Invalid JSON unicode '\u00ab' = no exception FAILED! expected: SyntaxError: Error #1132: Invalid JSON input. ecma3/JSON/invalid.abc : Invalid JSON unicode '\u00bf' = no exception FAILED! expected: SyntaxError: Error #1132: Invalid JSON input. ecma3/JSON/invalid.abc : Invalid JSON unicode '\u00Ab' = no exception FAILED! expected: SyntaxError: Error #1132: Invalid JSON input. ecma3/JSON/invalid.abc : Invalid JSON unicode '\u00bF' = no exception FAILED! expected: SyntaxError: Error #1132: Invalid JSON input. END FAILURES test run FAILED! Tests complete at 2011-04-18 21:55:22.116490 Start Date: 2011-04-18 21:55:17.081670 End Date : 2011-04-18 21:55:22.116490 Test Time : 0:00:05.034820 passes : 184 failures : 6 I absolutely understand what the first failure represents; I will work on that first thing tomorrow morning. But I do not understand what the remaining five failures are testing. Are these escape sequences that do not correspond to any valid code point? I just want to know what I am dealing with because that will help me track down where the fix belongs.

I forgot to include the dir.asc_args file in the last upload. All the previous notes still stand. (Except that I figured out how to do Vector.<Boolean>; that will be part of my next revision to Bug 640711.)

Comment on attachment 526901 [details] [diff] [review] suggested revisions to test suite I'll update the tests in a new patch. Thanks for the update. I am getting 191/191 passes now and 100% fn, 90% bc code coverage.

I hope to get to the review of the whole suite soon. But in the meantime, I did encounter issues with one test in particular: the Dictionary test that was proposed does not make much sense to me. (See email chain from April 13th for details on this.) Here's a suggested change.

Both this and attachment 527110 [details] have a dependency on attachment 527108 [details], as well as on the files in the Kraken benchmark: http://hg.mozilla.org/projects/kraken/file/tip/tests/kraken-1.1

added an AS3 JSON fuzzer. see README.txt in the zip.

Comment on attachment 526986 [details] [diff] [review] json tests (with 4/18 updates) R+. The issues below are all pretty easy to resolve. Let me know if you need more information about how to address item 8 below. Issues; 1. The parse("[") test case is commented out. The test should be re-enabled (I think the bug it was exposing has been fixed). 2. There is a stray XML object at the bottom of AS3Types.as; is that meant as a TODO for later? (I still need to check how Firefox handles analogous objects in its JSON library, as Lars suggested last week) 3. In Classes.as, you have spelled the TITLE as "Classs" with three 's'es. 4. What is the [ExcludeClass] metadata annotation? I did not put in any special handling for that, do I need to? 5. I already noted the Dictionary test issues in comment 10. The test you have for Dictionary should be replaced or simply removed; the one that is there does not really make sense. 6. On the Invalid.as tests, the explanatory messages for '1e' says "valid JSON numerical input" but I think you meant to write "Invalid" there. 7. On the Invalid.as tests, several of the explanatory messages say "parse invalid Array" when it is an Object being parsed. 8. On the large string tests: what you have is a good start. However, I realized this morning that it would be even better to have large string tests made up solely of characters that require multiple bytes to encode (preferably multibyte in both UTF16 and UTF8 encoding families, but UTF8 alone will serve us well enough for our testing for now, I think). In particular, I want to catch cases where breaking the string into blocks of fixed size is likely to break blocks in the middle of a character. I'm thinking a string made up of 10,000 3-byte characters would be a good start. 9. I'm a little surprised by the format of the tests 15.12.1-1-g4-1 through 15.12.1-1-g4-4; since all of the characters in the given strings are illegal, I would expect a parser to exit early upon encountering the first character in each string, and so the remaining elements of the string are not actually being tested. Are these some of the ones we inherited from ECMA? Anyway, I don't know if its really worthwhile to break them up into the set of 32 single character strings, but that's how I would have expected these tests to be presented. 10. The format of 15.12.1.1-g5-1 does not match the others, in that you have the AddTestCase within the Try block, rather than just putting the JSON.parse within the Try block (right)? 11. Typo: "non=hex" should be "non-hex" 12. Typo: "grammer" should be "grammar"

(In reply to comment #15) > 8. On the large string tests: what you have is a good start. However, I > realized this morning that it would be even better to have large string tests > made up solely of characters that require multiple bytes to encode (preferably > multibyte in both UTF16 and UTF8 encoding families, but UTF8 alone will serve > us well enough for our testing for now, I think). In particular, I want to > catch cases where breaking the string into blocks of fixed size is likely to > break blocks in the middle of a character. I'm thinking a string made up of > 10,000 3-byte characters would be a good start. My thinking here is that a string made up of 2-byte characters may not be too helpful since the block size is likely to be a power-of-two (or at least multiple-of-two). That's why I suggested 3-byte. That, and maybe another string made up of a sequence of ABABABAB... where A is a two-byte character and B is a three-byte character, that would probably be good.

(In reply to comment #15) > Comment on attachment 526986 [details] [diff] [review] > json tests (with 4/18 updates) > > R+. The issues below are all pretty easy to resolve. Let me know if you need > more information about how to address item 8 below. > One more issue I saw but forgot to mention in my review: 13. I don't think the "JSON.stringify('\n\f\\u0061')" test at the end of the JSON String tests is useful. Someone reading it might think that is testing that we stringify control characters correctly, but note that the backslash in front of the 'u' is itself escaped by a backslash, so you are just testing our ability to stringify \n, \f, a raw backslash, a 'u', and various digits. On a similar note, I'll be posting a regression test shortly for the bug that Bill found.

(In reply to comment #17) > (In reply to comment #15) > > Comment on attachment 526986 [details] [diff] [review] > > json tests (with 4/18 updates) > > > > R+. The issues below are all pretty easy to resolve. Let me know if you need > > more information about how to address item 8 below. Issue 14: Do these tests really belong in the ecma3/ directory? They are not part of the 3rd edition of ECMA-262, and some of the tests do not conform to or are simply not part of the 5th edition.

Added test of deep recursion, exposing a long-standing bug in my implementation. A simliar test for parse would be appreciated.

A small revision of the earlier form of the test: we still expect an exception to be thrown, so handle it. We just don't want a crash or a memory leak.

Added deep parse test. Going to stop adding tests now. dschaffe: please review and/or incorporate this work into the test suite, thanks!

just like attachment 527317 [details] but with calls to print(new Date()) removed. (I was timing the object and string creation loops to make sure they were not killing us. My original string creation loop was falling victim to O(n lg n) growth; doubling works better.)

Comment on attachment 527326 [details] regression tests file again, please review or incorporate into the test suite.

a few updates to the tests. I included the regress.as test

In regress.as changed loop counter for 1e5 to 1e3. It will be too slow in debug-debugger runs and especially on linux-mips and android.

minor fixes to JSON tests. includes Dictionary, ByteArray stringify. Also the XML return "{}" but the commented out test above returns "XML". For converted to ATS had to remove all print statements.

Comment on attachment 528096 [details] [diff] [review] json tests update 4/25 Looks fine. My experience with Bug 652200 is leading me to think that we should explicitly test that toJSON overrides work in the cases that we care about (i.e. Date, Dictionary, ByteArray), because it did *not* work for XML, so we should be checking the other cases in the meantime.

I created tests for toJSON on Dictionary, ByteArray, Date, and XML. Setting XML.prototype.toJSON does not work but all the other toJSON overriding and restoring worked.

Comment on attachment 528315 [details] [diff] [review] json tests for toJSON These tests are not broken, but they (IMO) also do not reflect what will be practiced by experienced JSON users. In particular, toJSON generally does not have to return a String. It will make much more sense for at least Dictionary.prototype.toJSON to return a freshly constructed object that mirrors the key/value mapping held in the dictionary (except where the keys in the mirrored object are strings rather than the actual objects). And it will be better to test that everything still *works* when such usage is exercised. :)

(In reply to comment #33) > Comment on attachment 528315 [details] [diff] [review] > json tests for toJSON I am not sure how to modify the patch to address the feedback. How would toJSON return an object and not a String? What would the code look like and since the ordering of objects is not deterministic from (for in) how would stringify() convert the object to a String? I was under the impression toJSON had to return a String and it's purpose is to serialize objects into something that makes sense as a String so stringify() can use toJSON.

(In reply to comment #34) > (In reply to comment #33) > > Comment on attachment 528315 [details] [diff] [review] > > json tests for toJSON > > I am not sure how to modify the patch to address the feedback. > > How would toJSON return an object and not a String? What would the code look > like and since the ordering of objects is not deterministic from (for in) how > would stringify() convert the object to a String? I was under the impression > toJSON had to return a String and it's purpose is to serialize objects into > something that makes sense as a String so stringify() can use toJSON. I'll post an illustrative patch.

Comment on attachment 528096 [details] [diff] [review] json tests update 4/25 Switching to R-: I tried to use the patch and discovered that runtests.py says this: [Compiler] Error #1046: Type was not found or was not a compile-time constant: Dictionary. ecma3/JSON/AS3Types.as, Ln 104, Col 11: var d:Dictionary=new Dictionary(); ..........^ 1 error found FAILED! file did not compile: ecma3/JSON/AS3Types.abc I think you left out an AS3Types.as.asc_args file, right?

dschaffe: I worked around the problem by cut-and-pasting from Classes.as.asc_args. Feel free to discard if you have something better, or just fold it into attachment 528096 [details] [diff] [review].

(In reply to comment #37) I may have forgotten to include the .as.asc_args in the patch. Yes it is the same as the Classes.as_asc_args but without the -md flag.

This patch is in response to comment 34. I, like Dan, originally thought when I first read the spec that toJSON() had to return a String. But that is not the case; toJSON just acts as an optional intermediary transformation on objects before they are passed to the next stage of stringification (namely the call to replacer, and then followed by the stringification traversal where it dispatches on the type of the result). Since it is just feeding into the later pass, it is legal for toJSON() to return anything that would have been legal inputs in the first place. (Its a fun exercise to explore the options here to expose ways to foil the cycle-detection algorithm.) Here I illustrate what I would expect to be one way for a client to provide application-specific handling of a Dictionary: by using application-specific state within the object keys to turn them into distinguishable keys and then returning the resulting object from toJSON.

(In reply to comment #39) > Created attachment 528353 [details] [diff] [review] > illustration of non String toJSON on Dictionary FYI: This patch (ie attachment 528353 [details] [diff] [review]) is meant to be applied on top of attachment 528315 [details] [diff] [review] ; it does not replace it.

Status? What work remains on this bug?

changeset: 6263:aac8713ffa57 user: Dan Schaffer <dschaffe@adobe.com> summary: bug 649800: updates to JSON testcases (r=fklockii) http://hg.mozilla.org/tamarin-redux/rev/aac8713ffa57

Felix and I agreed the best test coverage is to land https://bugzilla.mozilla.org/attachment.cgi?id=528096 + https://bugzilla.mozilla.org/attachment.cgi?id=528315 + https://bugzilla.mozilla.org/attachment.cgi?id=528332 + https://bugzilla.mozilla.org/attachment.cgi?id=528353 landed in TR and all tests pass.

For future consideration if there are problems with key ordering being non-deterministic notes from testplan: the key ordering in stringify on an object is non-deterministic: in the tests we have a function sortObject() in test/acceptance/shell.as to sort the keys a suggestion is to use the array replacer argument to set the key order