test(cli): introduce snapshot-based testing

[bajtos]

While working on #3754 and thinking about how to test #3688, I realized the current approach, where we write a bunch of regular expressions to match parts of the generated code, is no longer serving us well:

• The regular expressions are difficult to read. It's not clear how exactly does the generated code looks like.
• It's easy to write these regexes in such way that they don't catch significant changes (e.g. regressions).
• It's difficult to update the assertions when we improve some small aspects of the generated code.

In this pull request, I am proposing to introduce snapshot-based testing, as explained in Jest's guide.

• Implement basic snapshot matching in a helper file that can be later extracted into a standalone package
• Add few more test helpers specific for our CLI package
• Rework some of the integration tests for "controller" and "datasource" commands to use snapshots instead of regular expressions

As you can see, the snapshots are quite verbose. Because are tests are very coarse-grained and always generating the entire artifact (a controller file, a datasource class file), the snapshots are describing the entire artifact too.

While it may look like a bad thing, I think it's actually good - it's telling us that our tests are not well structured, we are missing unit-tests that can be focused on smaller parts of the artifact template (e.g. individual controller methods).

Discussion point(s):

• Should I update all tests to replace regular expressions with snapshots? I prefer incremental update to make pull requests easier to review.

Further thoughts:

• I tried to keep the format of snapshot files similar to what Jest produces, to simplify future migration from Mocha to Jest (if we ever get to do that).
• In the future, I'd like to explore snapshot-based testing for our code generating JSON & OpenAPI schema from models, I think this approach can be useful too.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• API Documentation in code was updated
• Documentation in /docs/site was updated
• Affected artifact templates in packages/cli were updated
• Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

[bajtos]

You may ask why I implemented our own snapshot matcher? The answer is simple - I could not find any package that would look reasonable & popular enough.

• The search: https://www.npmjs.com/search?q=mocha%20snapshot
• https://www.npmjs.com/package/mocha-jest-snapshots (forked from mocha-snapshots)
• https://www.npmjs.com/package/snap-shot-it
• https://www.npmjs.com/package/mocha-snapshots

[raymondfeng]

@bajtos Please fix lint errors:

/home/travis/build/strongloop/loopback-next/packages/cli/test/integration/generators/controller.integration.js
  247:10  error  'checkCreateContentsWithIdOmitted' is defined but never used. Allowed unused vars must match /inject|(\w+)Bindings/u  @typescript-eslint/no-unused-vars
  264:10  error  'checkCreateContents' is defined but never used. Allowed unused vars must match /inject|(\w+)Bindings/u               @typescript-eslint/no-unused-vars

[raymondfeng]

@bajtos +1 to support snapshot testing.

Is there any possibility to leverage modules from https://github.com/facebook/jest/tree/master/packages?

[jannyHou]

where we write a bunch of regular expressions to match parts of the generated code, is no longer serving us well:

+1 It's a pain to compare the regex and generated file, I used some tool online like https://www.debuggex.com/ when making changes in the controller generator. Thank you for coming up with the new approach.

The snapshot-based testing looks reasonable to me 👍 a question:

What would the failure report look like if the actual value doesn't match the snapshot? The current check(regex) doesn't provide very meaningful message, hopefully the new approach prints out more meaningful information.

[jannyHou]

nitpick: I feel a bit confused with the names here like what is "args 1" and "input 1", and why they are empty controller class. Maybe add some comments?

[bajtos]

The keys in the snapshots are generated from the test names, the number at the end is a counter to distinguish between multiple snapshots used by a single test.

Would it help to use a longer formatting? For example:

Suggested change

	exports[`lb4 controller basic controller scaffolds correct file with args 1`] = `
	exports[`lb4 controller basic controller scaffolds correct file with args -- snapshot 1`] = `

[agnes512]

I am still confused about the arg 1, input 1 here. Could you explain it with an example?

[bajtos]

Consider the following hypothetical test:

describe('lb4 datasource', () => {
  it('creates a TS and a JSON file', () => {
    // run a generator that creates two files
    expectFileToMatchSnapshot('src/datasources/db.datasource.ts');
    expectFileToMatchSnapshot('src/datasources/db.datasource.json');
  });
});

The are two snapshot-based assertions in a single test, therefore the key used to store & load the snapshot must somehow distinguish between these two.

The solution we (but also Jest and Tap) use: a counter that's increased with each snapshot assertion and that's included in the snapshot key.

In my example code above, the keys will be as follows:

1. creates a TS and a JSON file 1 and
2. creates a TS and a JSON file 2.

See the function buildSnapshotKey: https://github.com/strongloop/loopback-next/blob/77324c3c78530737893e69df75f4262fb149c161/packages/cli/test/snapshot-matcher.js#L107-L111

[jannyHou]

Nitpick: Would it be better to keep snapshots in separate files to make it more readable?

[bajtos]

When I first heard about snapshot tests, it was my first question - why are all frameworks keeping multiple snapshots in the same file? I would use a single file for each snapshot too!

Here are few reasons why it may be better to not keep snapshots in separate files:

• Ideally, snapshots should be used for unit-level tests where the snapshots are small, let's say 10-15 lines of code. In which case I feel it would be wasteful to have 20 files with 15 lines of code each, instead of a single file with ~300 LOC.

• I think there is also performance aspect in play. I did not measure it, but I expect that for large test suites like ours, the difference between reading 10 or 200 snapshot files can be noticeable.

• When there is 1:1 relation between a test file (e.g. controller.integration.js) and a snapshot file (e.g. controller.integration.snapshots.js), it's easier to navigate between these two files. I am not sure how often that's needed though, because snaphots are typically edited automatically by the test framework. 🤷‍♂

Because I'd like us to eventually upgrade from Mocha to Jest, I prefer to keep our snapshot files as close to what Jest produces as feasible. That way the migration won't have to touch all snapshot files, it might be super difficult to review such change to catch any unwanted changes!

[jannyHou]

Fair enough 👍 Good learning!

[bajtos]

Is there any possibility to leverage modules from https://github.com/facebook/jest/tree/master/packages?

It's possible but also rather difficult. Many parts of jest snapshots are tightly coupled with other parts of the testing framework. I am also concerned about effort needed to keep our jest-based code in sync with newer versions of jest. Because jest-snapshot is not really meant to be consumed by 3rd-party packages, they offer very little API stability guarantees.

Their snapshot engine is also very powerful. For example, they leverage Prettier and Babel to edit the actual test code to allow in-line snapshots. Such complexity makes me even more reluctant to integrate with jest-snapshot.

Eventually, I'd like us to migrate from Mocha to Jest, but that's a longer story...

[bajtos]

What would the failure report look like if the actual value doesn't match the snapshot? The current check(regex) doesn't provide very meaningful message, hopefully the new approach prints out more meaningful information.

I modified one of the snapshots and introduced a space into the schema title name (inspired by #3754). Below is a screenshot of the reported test failure, it looks very easy to read to me 👏

We can improve the error messages in the future, for example Jest prints pretty verbose details as can be seen in https://jestjs.io/docs/en/snapshot-testing

[bajtos]

@raymondfeng @jannyHou thank you for a great feedback! let's keep the discussion going 👍

[jannyHou]

Thank you @bajtos for the screenshot in #3756 (comment)! MUCH better.

[agnes512]

I definitely like this idea!

1. It's painful to check the content with regex ._.
2. instead of checking file content line by line assert.fileContent(..), it is easier to write/maintain snapshots tests, and it looks more 'complete'.

Besides the effort to pick up snapshot test/ rework on tests, any other down side of the solution it might have?

[agnes512]

I am still confused about the arg 1, input 1 here. Could you explain it with an example?

[bajtos]

Besides the effort to pick up snapshot test/ rework on tests, any other down side of the solution it might have?

Great question! I wish we were asking it more often :)

My main concern is about over-specified assertions. If we are not careful to keep the test cases and snapshots narrowly focused, then each snapshot will include not only the data that's essential for the test, but also a lot of extra unrelated content. For example, in a test verifying how we generate model-data type and schema for create method, we would ideally want the snapshot to cover only the create method with decorators. At the moment, our snapshot contains the entire controller class. This creates two problems:

1. It's difficult to see which parts of the snapshot are important & relevant to the test and what's just a boilerplate. For example, when testing recently introduced feature that allows create method to support models where the id is supplied by the user, the important part is whether data argument has type Omit<Product, 'id'> or Product. This is difficult to spot when the snapshot contains the entire controller class.
2. Changes unrelated to the tested scenario are going to trigger snapshot updates. For example, when changing implementation of the controller method deleteById, we will have to update all snapshots including those for create tests.

When you combine these two problems, it's difficult to review pull requests well - there are too many snapshot changes (because of 2) and it's difficult to tell which of them are harmless and which require detailed consideration (because of 1). It's easy to introduce a regression that we won't notice, because the snapshot change that shows the problem is buried in too much noise.

The problem of over-specified assertions is not unique to snapshot testing, it's just that snapshots make it much easier to write such poor tests.

As the saying goes, with great power comes great responsibility. We must be more careful to keep our tests well written and avoid the temptation of taking the easy path.

[hacksparrow]

Looks good, overall. Just a concern:

How about situations where we can check with apple|orange in regex? How would snapshots handle this?

[bajtos]

How about situations where we can check with apple|orange in regex? How would snapshots handle this?

I assume that if you check with /apple|orange/ then you have at least two test cases, where some tests end up with apple and other with orange.

When using snapshots, you have one snapshot for each assertion (and for each test, if the assertion is called from a helper function). Snapshots for tests using apple will contain apple, snapshots for tests using orange will contain orange.

That's why it's important to keep the snapshots small, as I explained in my previous comment.

[bajtos]

FIXME: This code is writing snapshot files in series using sync FS, which can be unnecessary slow. Let's rewrite it to use async and write tests in parallel via Promise.all.

[bajtos]

See #3776

[bajtos]

FIXME: when this assertion fail, we should tell the user how to fix the snapshots via UPDATE_SNAPSHOTS=1.

[bajtos]

See #3776