Skip to content

Tests refactor#257

Merged
kddnewton merged 51 commits intomasterfrom
tests
Apr 17, 2019
Merged

Tests refactor#257
kddnewton merged 51 commits intomasterfrom
tests

Conversation

@kddnewton
Copy link
Member

Okay so this is a big one. A couple of reasons for it:

  • After seeing @johnschoeman's work on Jts ct ruby specs #209 and Jts blocks mini test #210, I wanted to make it easier to work in a test-driven manner.
  • The test suite needed to be sped up. It was kind of all over the place and doing too many things.
  • The tests were testing a lot of the same things in different places, and so weren't very reliable.

Here's what this PR does:

  • Migrates all of the existing node tests into new unit tests in jest.
  • Maintains a single parser child process so that it's much much faster to run (the full test suite is now around 10s). I actually think this may be a potential improvement for the parser in general (it's possible we could migrate this over to just be the regular parser).
  • Remove the snapshots. At the end of the day there were too many to check and they were getting in the way. I really want to bring these back, but only in a full integration test capacity.

I want to write up some docs and start incorporating some of the open PRs into this work, but I figured I'd open this up now.

@kddnewton kddnewton mentioned this pull request Apr 13, 2019
@localhostdotdev
Copy link
Contributor

Remove the snapshots. At the end of the day there were too many to check and they were getting in the way. I really want to bring these back, but only in a full integration test capacity.

was curious about this, here is what I found useful to understand:

toMatchFormat: (format, config = {}) => checkFormat(format, format, config)

const checkFormat = (before, after, config) => {
  const options = Object.assign({}, { parser: "ruby", plugins: ["."] }, config);
  const actual = prettier.format(before, options);

  return {
    pass: actual === `${after}\n`,
    message: () => `Expected:\n  ${after}\nReceived:\n  ${actual}`
  };
};

ok, so it's checking for idempotence, quite nice

Copy link
Contributor

@johnschoeman johnschoeman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks great to me, and is an impressive amount of work! I had a question, if you have a moment to answer. Let me know if you want any help finishing the expectations in some of the tests.

(hasBody || variables) ? " " : "",
variables ? path.call(print, "body", 0) : "",
path.call(print, "body", 1),
" }"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

});

expect.extend({
toChangeFormat(before, after, config = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like being able to be explicit about the config here!

@@ -0,0 +1,74 @@
const { spawn } = require("child_process");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me. Just to make sure I'm understanding correctly, the reason you are going with jest here as opposed to rspec or minitest is for performance reasons? or is there something else that makes jest preferable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple of reasons. The #1 is speed - shelling out to ruby -c and rubocop and minitest takes a ton of time. I'd still like to bring that back, but only in an integration test capacity that doesn't need to run for the whole unit test suite. #2 is test coverage. It's really hard to measure test coverage when we're using ruby specs versus just using the stuff built into jest. #3 is the convenience of jest itself (test.only, describe.each, etc.). There's a lot of stuff baked in that makes it pretty nice to work with.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. Thanks for the explanation.

@kddnewton kddnewton merged commit a608661 into master Apr 17, 2019
@kddnewton kddnewton deleted the tests branch April 17, 2019 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants