d3x0r / JSON6

JSON for Humans (ES6)
Other
236 stars 14 forks source link

- Testing: Add test for trailing comma #33

Closed brettz9 closed 4 years ago

brettz9 commented 4 years ago

Adds a test for trailing commas.

However, this test is currently failing (and shouldn't).

It's adding to the expected object as follows:

{
+        "a": {
+          "a": 5
+        }
         "abc": {
           "a": 5
         }
       }
brettz9 commented 4 years ago

Note that getting a fix for this should also fix the package.json issue mentioned at the end of the original post of #31.

d3x0r commented 4 years ago

json6.js | 100 | 98.89 | 100 | 100 | 160,167-175,634-636

these are unreached conditions... while I trust that they haven't gotten there, and I haven't figured a test to trigger them; I don't yet trust they are constant. Will look at this test.

d3x0r commented 4 years ago

Included tests for this with badTest... and fixed handling that. do you still want this merged?

brettz9 commented 4 years ago

Great re: expanded coverage, thanks!

Are you thinking instead of the octal change? I don't see any tests for trailing comma, and it is still failing.

brettz9 commented 4 years ago

Btw, as far as lines, 634-636, I've added some coverage in #36. However the check at the end, if( status ) val.value_type = VALUE_STRING;, I don't see how status can be false, as that only occurs if an error has been thrown afterward. While we can catch the error in the calling script, I'm not sure at this point if it is possible, and how, it could back to that check.

d3x0r commented 4 years ago

That sounds about right; that's leftover from the initial c version that didn't throw, but instead always gracefully returned. Was sort of looking that status is kind of not meaningful; although in the case of stream processing... if a packet is bad, subsequent packets should be skipped until the parser is reset().... so it does have SOME meaning. Edit: https://github.com/d3x0r/JSON6/blob/master/test/json6BadTest.js#L279-L289

brettz9 commented 4 years ago

Ok, thanks. I've added a commit to remove the status check.

Re: the bad test reference--those tests are for no comma--the issue here is with a trailing comma.

d3x0r commented 4 years ago

okay; I did some digging into this, it does VERY strange things....