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

add create mode #60

Closed
wants to merge 1 commit into from
Closed

Conversation

mildfuzz
Copy link

No description provided.

@@ -45,6 +45,7 @@ const maskImgWithRegions =

export type CompareOptions = CompareImagesOpts & {
maskRegions: MaskRegions
mode: 'create' | 'failFast'
Copy link

@GrayedFox GrayedFox Aug 19, 2024

Choose a reason for hiding this comment

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

I think it's worth zooming out a bit here and deciding whether or not the pdf-visual-diff tool should be the thing that handles throwing/failing a test, or if it should instead expose some functionality that the test runner (mocha, jest, Cypress, etc) can plug into.

If applying the single responsibility principle to the configuration option I think it might make sense to have two distinct, separate boolean values that can be on/off in any combination. Forgive the names, likely some clearer and simpler variable names that could be used, just pressed for time at the moment, example uses JSON schema syntax:

{
  "options": {
    "generateIfSnapshotMissing": {
      "type": "boolean",
      "description": "Whether or not generating a snapshot should be done if one is missing",
      "default": true
    },
    "failIfSnapshotMissing": {
      "type": "boolean",
      "description": "Whether or not a missing snapshot should result in a failed check",
      "default": false
    }
  }
}

This way people are free to use them in any combination, i.e. if generating a snapshot is unexpected (due to it being part of a normal run) pdf-visual-diff will see there's no matching snapshot, fail the comparison, and the resulting boolean can be used by the test writer to act on.

Assuming the above was implemented and taking advantage of these modes @mildfuzz would this come close to your desired behaviour? Example uses node environment variables but could also be done programatically:

//package.json
{
  scripts: {
        generatePdfs: "PVD_FAIL_IF_SNAPSHOT_MISSING=false PVD_GENERATE_IF_SNAPSHOT_MISSING=true jest ./specs/ || false",
        test: "PVD_FAIL_IF_SNAPSHOT_MISSING=true PVD_GENERATE_IF_SNAPSHOT_MISSING=false jest ./specs/",
    }
}

In our case we have multiple PDF checks done in a single context() block so allowing the test runner to continue and not fail that block in order to continue to generate PDFs is useful for us, even though when we do generate PDFs we want to output an non-zero status code when all tests pass anyway, which we do by fudging the exit code using the || false bash trick 🙈

Copy link
Owner

Choose a reason for hiding this comment

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

I like this proposal.
@mildfuzz do you think it would be useful to you as defined above by @GrayedFox?

I could also wing it as well.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I could certainly do what I need to with this set up, and if it adds further flexibility, all the better

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, I could certainly do what I need to with this set up, and if it adds further flexibility, all the better

Great! Are you still planning to finish it, or would you prefer if I took it from here? No worries either way, just let me know what works best for you!

Copy link
Author

Choose a reason for hiding this comment

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

I'm unlikely to get free time for this for a week or so, so if you're keen I would be happy for you to take it over

@moshensky
Copy link
Owner

Closing in favour of the other PR that was merged.

@moshensky moshensky closed this Sep 26, 2024
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