feat: add allowSnapshotCreation and failOnMissingSnapshot to comparison logic

[moshensky]

Supersedes #60

[moshensky]

@mildfuzz, @GrayedFox,

Have a 👀 please.

[GrayedFox]

Minor comments but nothing blocking

[GrayedFox]

@moshensky these specs look like they cover the new options and functionality but I was wondering if you think we should also check the interaction with the existing approve and discard CLI commands?

From what I can see the approve and discard CLI command module is entirely isolated from the compare pdf command, which would be run as part of a test suite, so it shouldn't be an issue but I'm a bit unclear on what happens if allowSnapshotCreation is false and there's an existing snapshot, I assume the .diff.png would be created but not the .new.png?

If that's so maybe dropping a hint in the readme that to use the test:pdf-approve command allow snapshot creation needs to be true while running specs.

Edit: removed end of sentence which wasn't logical.

[moshensky]

You correctly noticed that the CLI commands are separate from the comparePdfToSnapshot logic. I added them to assist with replacing existing snapshots after changes are made to the code being tested by comparePdfToSnapshot. The commands are straightforward: they traverse the file system for snapshots, and approve replaces existing snapshots with the .new version and deletes the .diff. I don't really use the discard command; I just use git reset.

Therefore, the CLI commands don’t execute anything beyond their own functionality and do not invoke comparePdfToSnapshot in any way. That said, the CLI command documentation probably needs improvement.

[GrayedFox]

is the negation here intentional? Probably just me being an annoying linter, this might be a bit clearer regarding intention :)

const snapshotExists = existsSync(snapshotPath);
const failTestDueToMissingSnapshot = (!snapshotExists && failOnMissingSnapshot);
const shouldGenerateSnapshot = (!snapshotExists && allowSnapshotCreation);

if (shouldGenerateSnapshot) {
    return pdf2png(pdf, pdf2PngOptions)
      .then(maskImgWithRegions(maskRegions))
      .then(writeImages(snapshotPath))
      .then(() => failTestDueToMissingSnapshot)
}

return pdf2png(...);

I think that achieves the same but it's untested - thanks for the continued work on this project, tag me any time for reviews, happy to contribute when/where needed too.

[moshensky]

Won't this change the semantics a bit?

When the function is executed, it has the following side effects:

• If a previous snapshot file does not exist:
  • If allowSnapshotCreation is true (default), the PDF is converted to an image,
    saved as a snapshot, and the function returns true.
  • If allowSnapshotCreation is false and failOnMissingSnapshot is true,
    the function returns false without generating a new snapshot.
  • If allowSnapshotCreation is false and failOnMissingSnapshot is false
    (default), the function returns true without generating a new snapshot.
• If a snapshot exists...

This means that failOnMissingSnapshot only applies when allowSnapshotCreation = false, which clearly deviates from your original intention of keeping these two options independent.

If they are supposed to be independent, what should happen when a snapshot does not exist and both allowSnapshotCreation = true and failOnMissingSnapshot = true?

Let's assume comparePdfToSnapshot behaves as follows when no snapshot is present:

#	allowSnapshotCreation	failOnMissingSnapshot	Behavior
1	true	true	Creates snapshot and returns false
2	true	false	Creates snapshot and returns true
3	false	true	Does not create a snapshot and returns false
4	false	false	Does not create a snapshot and returns true

In the first case #1, on the next run this code path won't be triggered, because a snapshot will exist, and comparePdfToSnapshot will return true instead of false as it did in the previous run.

In the second case #2, the code path also won't be triggered on the next run due to the previously created snapshot, but at least the function will return the same result (true).

I prefer consecutive runs to always return the same result (excluding side effects) unless the PDF has changed in the meantime. That’s why the current implementation behaves as it does.

Ultimately, what problem are we solving here? I see a clear case for having a "CI" option that returns false when a snapshot doesn't exist. This could ensure that, during a CI test run, any new test also has an associated snapshot committed. If this is the only use case, we could simplify the options significantly.

[moshensky]

@GrayedFox, @mildfuzz,

Can I have your input?

[mildfuzz]

I'm happy with the implementation as is

[GrayedFox]

You're right, tying the result of returning false only if a snapshot is missing was a mistake (hasty writing on my part).

I think this keeps original intention as well as adheres to making the runs idempotent(ish) in terms of consecutive runs having the same result, excepting the side effect of creating a snapshot if one is missing.

Please take all this with a grain of salt! Am just thinking about how to make the code a bit more declarative for my future eyeballs, from what I can see the current implementation on this PR meets all the criteria and desired behaviours ✅

[moshensky]

@GrayedFox, thank you for the code improvements. I'd like to postpone them and discuss them as a follow-up to this PR. I prefer to have a separate refactoring PR without any embedded API changes.

Regarding the API itself, I become more convinced that we should add only one new option: failOnMissingSnapshot which would be optional and set to false by default. This will enable the CI use case where comparePdfToSnapshot returns false when the reference snapshot does not exist.

@mildfuzz, will that suit your use case?

Having two boolean variables, with potential four different states - where one is ignored by the code and the other might not be used at all - makes me want to simplify things. You can see here what I mean.

[GrayedFox]

The fail on missing snapshot meets our immediate needs for implementing this in our CI, if @mildfuzz feels the same, happy to see just that functionality merged in the other PR and we can think a bit deeper about adding the second option in a way that keeps everything intuitive and aligns with the core design goals and principles of your module

[moshensky]

I'll move forward with merging just failOnMissingSnapshot functionality.
We can revisit adding any additional options later.

[GrayedFox]

You're right, tying the result of returning false only if a snapshot is missing was a mistake (hasty writing on my part).

I think this keeps original intention as well as adheres to making the runs idempotent(ish) in terms of consecutive runs having the same result, excepting the side effect of creating a snapshot if one is missing.

Please take all this with a grain of salt! Am just thinking about how to make the code a bit more declarative for my future eyeballs, from what I can see the current implementation on this PR meets all the criteria and desired behaviours ✅