Decaffeinate Tests

[curran]

Closes #7

[curran]

At this point, this is a rough first pass at the change, presenting the raw output from the automated tool https://github.com/decaffeinate/decaffeinate .

Looking through the resulting code, the added verbosity may benefit from a Prettier pass (e.g. some lines are way too long now).

[curran]

I ran prettier --single-quote ./*.js --write in the tests directory, which helped a lot.

[curran]

The remaing tasks here are:

• Do a manual pass over all the generated JS files and clean them up. The generated decaffeinate suggestions comments may be helpful with this.
• Investigate why the immutable guarantees tests are failing in CI (all other tests pass).

I'm willing to do this last leg of work as well, but I'd like to hear from you @josephg on the general approach of this PR. What do you think? Would this be a welcome change? Thanks!

[josephg]

Yep this looks great! I do appreciate this change, and like you I'm curious why the tests are failing. I'd also like it if decaffinate didn't add semicolons at the end of all my lines (my poor hipster JS style conventions...). Once you sort that out I'll merge the change.

A better way to verify immutability would be to recursively call Object.freeze on all inputs to all the functions in all tests. The nice thing about that approach is that if any tests inappropriately modify data, the stack trace will point to the exact source of the error. If we do that, we probably shouldn't need special immutability test cases at all.

[curran]

Awesome! Thanks for reviewing. I will work on the points you mentioned.

I can tell Prettier not to include semicolons, so that change at least will be automatic.

Changing the way that the immutability tests work at first glance feels out of scope for this PR, but maybe it's necessary now if the decaffeination changed something subtle/fundamental with those particular tests.

[josephg]

Yes I agree it’s out of scope. And I’d like to know what is causing the problem anyway, on the off chance it’s independently something worth fixing.

[curran]

For the record, this is the prettier command I used:

prettier --single-quote --no-semi ./*.js --write

Also, the tests pass locally. They only fail in CI, which suggests the failure is possibly related to some behavior that changed across Node.js versions. I'm on Node 11, and CI is on Node 10. Although, when I use Node 10 locally, the tests pass. The error was to do with module resolution. Perhaps if I update the lock files...

[curran]

Ok I found the cause of the error.

The test uses a require call to pull in a .coffee file. In test/genOp.js:

194         const genString = require('ot-text/test/genOp') // Tries loading genOp.coffee

This should work if we bring back the coffeescript/register in mocha config.

It would not be a total decaffeination, but it's pretty close.

[curran]

Woohoo CI passes! Onto the manual cleanup.

[curran]

@josephg Ready for final review. I did a manual pass through the code and removed unnecessary stuff added by decaffeinate (mostly unnecessary return statements and IIFEs). I left your commented out log statements in there (and most of the comments for that matter I left untouched), as I figured they may be useful as part of your workflow.

I would like to bring your attention to this line in the original test, which doesn't seem to make sense

json1/test/test.coffee

Line 2003 in d8c998d

expect: [ [ 'a', { r: true }, 0, { p: 0 } ], [ 'b', { d: 0 } ] ]

.

      expect: [ [ 'a', { r: true }, 0, { p: 0 } ], [ 'b', { d: 0 } ] ]

I've added a TODO flag for you on the code generated from that line, right here https://github.com/ottypes/json1/pull/8/files#diff-c1129c8b045390789fa8ff62f2c6b4a9R2781

      // TODO have a look at this - doesn't seem right to just define an object
      {
        expect: [['a', { r: true }, 0, { p: 0 }], ['b', { d: 0 }]]
      }

Thanks for accepting my contribution!

[curran]

By the way, I manually changed a coupld of these if else sections to use these ternary operators instead. Happy to switch them back if you'd prefer.

[josephg]

I would like to bring your attention to this line in the original test, which doesn't seem to make sense

json1/test/test.coffee
Line 2003 in d8c998d
expect: [ [ 'a', { r: true }, 0, { p: 0 } ], [ 'b', { d: 0 } ] ]
.

      expect: [ [ 'a', { r: true }, 0, { p: 0 } ], [ 'b', { d: 0 } ] ]

Yeah I have no idea what thats doing there. Its a remnant of some test case, no doubt. Its safe to delete. In a sense the real check for correctness is the fuzzer. The unit tests exist to give an overview of the behaviour in all cases; and because if code changes break the existing tests, its much easier to figure out the mistake by looking at some cleaned up failing test cases rather than poring over new logs out of the fuzzer.

I'm not entirely happy that coffeescript is still a dependancy after this change - though I assume we'll be able to remove it with a bit more refactoring.

But yeah, great stuff! Thanks for your work on this - I really appreciate it. Merging.

[curran]

Hooray!