search

d3x0r / JSON6

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

Mocha nyc #24

Closed brettz9 closed 4 years ago

brettz9 commented 4 years ago

Builds on #23

One fix I wanted to make but didn't (without hearing your feedback) is to add eslint, or at least to make consistent tab/space usage.

Note that this PR reveals there are, as I see it, at least four failing tests (assuming the expect statements reflected your intent as I tried to do):

  1. json6BadTest.js:12 - true is not failing as an object key, though your test seemed to suggest it should fail.
  2. json6BadTest.js:54 - For what should have been a bad value, { a : 'no quote' [1] }, this was received instead: { a: [ 1 ] }.
  3. json6StreamTest.js:40 - Some values were missing when the stream pushed parser.write( 'true false null undefined NaN Infinity' ); (I figured all of these were supposed to make it.)
  4. json6Test.js:126 - In "Basic parsing" -> "Objects" -> "Single-quoted key value with backslash, carriage return, and newline:", an extra letter got stripped out after a backslash, carriage return, newline.

(There may of course be more bugs revealed by bringing up coverage from the current 75% statement coverage.)

Note that I have added Mocha 3, and changed your tests so that they are compatible with the old Node versions your .travis.yml file indicated you were supporting (e.g., removing a few arrow functions and ES6 templates (though I chose to avoid strings in favor of objects which I regular JSON.stringify'ed to take advantage of out-of-the-box IDE syntax highlighting)).

If you'd prefer to drop that old support, I could make the change in .travis.yml and in the engines property I added to package.json.

I did not add nyc in the test script because the latest version I've installed won't work in the older Node environments.

And finally, the reason I moved files from tests to test was so that the test script could use Mocha's default of checking a test folder, while also allowing the same script to be used to pass in individual files, e.g.,

npm test -- test/json6Test.js

...will also work.

d3x0r commented 4 years ago

This is quite the change. I'll get back to this, most of it looks good

d3x0r commented 4 years ago

:) could have wished you made these more distinct... I'm sure the other PRs aren't so big once this is merge so...