Migrate from snapshot tests to assertion tests

[ehmicky]

We use snapshot tests in Netlify Build, which have some pros but also the following cons:

• This is a rather new testing methodology that might be unfamiliar to many developers
• Ava's implementation of snapshots require two snapshot files (one human-friendly, one machine-friendly) to be persisted, which results in some confusing developer experience
• Snapshots must be identical between machines, which require us to normalize them with a big list of regular expressions. This normalization is brittle, must be constantly maintained and removes some potential useful information

Instead, I think we should move back towards assertion tests. For example, in @netlify/config, instead of making a snapshot of the return value, we should return it to the test function and let the test perform some assertions.

Note: there are more than 500 tests.

What do you think @erezrokah?

[erezrokah]

I don't think we need to migrate all tests, just the ones where it makes sense. I do find it harder to know what the test is asserting with snapshots as you need to examine the entire stringified output. On the other hand, adding new tests is much easier when using snapshots as you don't need to explicitly write assertions. I would:

1. Use snapshots when the test name and output clearly show what is being tested.
2. Use assertions when better granularity is required.

[ehmicky]

This makes sense.

One of my main concerns is to make it inviting for new team members to contribute new tests with the existing setup.

If a new team member does not understand the test setup, it is most likely that they would fall back to creating some unit tests for the specific logic they are adding. I think this would be a step backward in terms of having a comprehensive yet easy to maintain test setup for this repository:

• This would split the tests setup into different methodologies, each requiring its own maintenance and utilities
• Performing tests at an integration level has worked well with this repository. I am concerned lower-level unit tests would couple tests with implementation details. It also gives a lower confidence into the logic actually working correctly for the end user.

Instead, I would like to make sure the current setup fits the preferences of new team members and I was wondering whether test snapshots might be an obstacle. Several people have now complained about not finding this testing methodology intuitive for them.

One thing we could do is to add test helpers to make it easy to opt-out of snapshot testing and do assertion testing with the current setup. We could experiment with it, what do you think?

[erezrokah]

We could experiment with it, what do you think?

Sure that would work. You were thinking about adding an argument to runFixture? Something like that?

[ehmicky]

Yes, and also ensure the return value of runFixture() can be used for assertion purpose. For example, the return value of runFixture() in @netlify/config is currently a string (the config JSON-serialized), not an object.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had activity in 1 year. It will be closed in 14 days if no further activity occurs. Thanks!

[github-actions[bot]]

This issue was closed because it had no activity for over 1 year.