Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Snapshot testing #393

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Snapshot testing #393

wants to merge 10 commits into from

Conversation

jiripudil
Copy link
Contributor

  • new feature
  • BC break? no
  • doc PR: todo

Snapshot testing is a technique which allows users to instruct the testing framework to generate and store a snapshot of a tested value and in the subsequent runs assert that the value matches the snapshot. It presumably gained popularity among web developers because it was introduced in Jest to facilitate testing of the user interface. On backend, it is particularly useful e.g. for testing API responses.

I'd like to explain in short some of the design decisions – first and foremost, why this can't be done in an external code similarly to PHPUnit plugins: Tester runs tests differently. Where in_array('--update', $argv) is sufficient in PHPUnit and works even in third-party code, Tester requires changes in the test runner to pass the information to the child processes.

I added the --update-snapshots flag and an environment variable. In the updating mode, tests with new or updated snapshots fail. I initially skipped them, but then I changed my mind: PHPUnit plugins mark such tests as incomplete, which is a PHPUnit-specific concept and is somewhat different from skipped tests. I think that in Tester, failing the test is semantically more correct than skipping.

As for the assertion API, I started with a simple Assert::matchSnapshot() method, but the code eventually grew with the reading and updating logic, so it started to make sense to extract it to a separate class: Snapshot. I am considering adding the method back to Assert as an alias so that the Assert class remains to be the primary public API, but I don't have a strong opinion on this.

The Snapshot::match() method requires a snapshot name. I played with generating it from test arguments, but I couldn't figure out a way to make it deterministic in all cases, especially accounting for the fact that @testCases can be executed as a single test as well as each method in a separate job. In the end it was much easier to leave it to the developer. The only problem is that the snapshot name is used as a part of the snapshot file name, so I guess it should be validated against a limited set of characters – or at least it should be well documented that it has to be a safe string. What do you think?

The last thing to point out is that var_export and eval are used for storing and reading the snapshot value. I tried other solutions such as JSON or serialize and while var_export also has its trade-offs, it still seems to be the best one: it doesn't change x.0 floats to x integers like JSON does; it produces PHP code and as such is instantly readable, as opposed to serialized values. The var_export function is bad at serializing objects, but objects do not typically need to be stored in snapshots. I guess that as long as this limitation is documented, it should be good enough. It's also what the aforementioned PHPUnit plugins use.

@milo
Copy link
Member

milo commented Oct 29, 2018

I really like this idea. It's about year and half I created something similar in my local repo. But as a Tester's plugin and on high layer that this. Actually, I didn't hear about snapshot testing till today. So thanks :)

To issues you mentioned...

It's quite long in my mind that the Tester could support "user's arguments" from CLI. Some UNIX tools (based on getopt, AFAIK) use -- (end of args) separator: my/bin --args -- anything --else. The -- stops arguments processing and the rest of line takes as ordinary values. The Tester could pass it to a test. This solves some cases when users want to somehow control tests behaviour and they are using env variables for it now. But with snaphost testing in the Tester core, the global argumnet is more suitable.

The var_export() is OK. Value can be stored as <?php return ...; in a snaphost-xy.php so evil() is not needed.

I have to think about the rest. But definitely 👍

@jiripudil
Copy link
Contributor Author

jiripudil commented Oct 30, 2018

It's quite long in my mind that the Tester could support "user's arguments" from CLI.

👍 that would greatly improve extensibility. It's basically the only reason why snapshot testing can't be made as an external plugin without resorting to env variables.

so evil() is not needed.

I believe eval is used because it is more reliable than include or require. With include, there is no way to distinguish whether false means a failure, or a genuine false is stored in the snapshot, and require needs more complicated error handling, while eval just returns the expression (or throws ParseError) and the IO error handling is left to file_get_contents where it is easier to do. But perhaps when preceded by is_readable check, require should be equally suitable.

Looking forward to what you have to say to the other points!

@dg
Copy link
Member

dg commented Oct 30, 2018

Failure can be distingushed by triggered error.

@milo
Copy link
Member

milo commented Nov 12, 2018

Thinking about API, just for discussion.

Not sure, there should be class Snapshot in user space. Looks like Assert::snapshot() is sufficient. IMHO the method should assert always as Assert::same(), not match().

Signature of method could be Assert::snapshot(string $name, mixed $actual) (in manner of ($expected, $actual)). So, user have to pass snapshot name explicitly, no need for automatic naming. Every snapshot can be saved only once to prevent rewriting. That means, one snapshot can be asserted only once. Following is not allowed:

Assert::snapshot('name', $foo);
Assert::snapshot('name', $bar);

What do you think?

@jiripudil
Copy link
Contributor Author

Not sure, there should be class Snapshot in user space. Looks like Assert::snapshot() is sufficient.

@milo that was something I was considering too. I've tried the suggested approach and it looks really good.

Does anybody have any idea why the test fails on appveyor?

@milo milo force-pushed the master branch 2 times, most recently from eaaeb7b to 7184606 Compare February 21, 2019 06:47
@dg dg force-pushed the master branch 2 times, most recently from ee525b7 to 6ca248e Compare February 27, 2019 13:34
@jkuchar
Copy link
Contributor

jkuchar commented Sep 3, 2019

Does anybody have any idea why the test fails on appveyor?

E_WARNING: file_put_contents(C:\projects\tester\tests\Framework/snapshots\Assert.snapshot.update.updatedSnapshot.phps): failed to open stream: Permission denied

Try to have path to phps file shorther then 255 chars.

@jiripudil
Copy link
Contributor Author

Try to have path to phps file shorther then 255 chars.

This one is 88 characters long. I've even made it use platform-specific directory separator, but it doesn't seem to help. Unfortunately I don't have any experience with appveyor and not much with Windows either, so it's a mystery to me 😟

@dg dg force-pushed the master branch 7 times, most recently from 37f5151 to c3bd1b2 Compare November 19, 2019 13:19
@jiripudil jiripudil force-pushed the snapshot-testing branch 4 times, most recently from 4ae29b9 to 0899819 Compare January 15, 2020 12:30
@jiripudil
Copy link
Contributor Author

Ha, I've accidentally found why appveyor failed. It's not a feature, it's a bug, apparently not fixed until PHP 7.2.18 via this commit.

I'm now skipping the test in environments where this bug is manifested and everything is finally green 🍏

@dg dg force-pushed the master branch 4 times, most recently from a2312d9 to 84506f2 Compare January 21, 2020 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants