Skip to content

✨ Add per-snapshot config validation!#416

Merged
wwilsman merged 1 commit intomasterfrom
ww/per-snapshot-config-validation
Jul 15, 2021
Merged

✨ Add per-snapshot config validation!#416
wwilsman merged 1 commit intomasterfrom
ww/per-snapshot-config-validation

Conversation

@wwilsman
Copy link
Copy Markdown
Contributor

@wwilsman wwilsman commented Jul 15, 2021

What is this?

While working on #305, the decision was made to have the config schema for snapshot options included in @percy/core since that is where the options are consumed. Initially however, core only registered the schema while the snapshot command used it to validate snapshot listing options.

With #406 refactoring the merge and validate utils, the getSnapshotConfig helper can now be refactored to utilize their new features to better merge and now validate options! This brings config validation to all of the SDKs as warnings logged whenever percySnapshot is called with unrecognized or invalid options.

Speaking of invalid options, this PR also adds more restrictions to existing config options:

  • The widths and minHeight options have min and max values. These values are derived from the upload command clamping, which itself was derived from our API restrictions.
  • Hostnames in allowedHostnames must be actual hostnames, meaning no protocols and no paths. Including protocols and paths never worked and this validation will help clear up that confusion.
  • The networkIdleTimeout should never really be larger than 500ms, but is restricted to 750ms to give it some breathing room. This restriction is imposed because long timeouts cause slow tests and don't actually address the underlying network issues.
  • Discovery concurrency has a minimum value of 1 because having a negative or zero value causes the queue to lock up.

For new per-snapshot validations other restrictions are also imposed:

  • Disallowed capture options, like waitForSelector, when performing discovery for an existing domSnapshot.
  • The waitForTimeout option must be between 1ms and 30000ms.
  • The execute option must be a string or function.
  • For additionalSnapshots, a name, prefix, or suffix is required.
  • For additionalSnapshots, a prefix or suffix cannot be provided with a name.

Tests were updated accordingly and a couple things that were previously taken care of in the config helper are now taken care of where necessary, such as defaulting an additional snapshot name using a prefix/suffix.

This will close #389

@wwilsman wwilsman added the ✨ enhancement New feature or request label Jul 15, 2021
@wwilsman wwilsman requested a review from Robdel12 July 15, 2021 16:37
Copy link
Copy Markdown
Contributor

@Robdel12 Robdel12 left a comment

Choose a reason for hiding this comment

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

🏁 I am in love. The network idle timeout & allowed hostnames validations are going to be so good going forward

@wwilsman wwilsman merged commit 3c0c0bd into master Jul 15, 2021
@wwilsman wwilsman deleted the ww/per-snapshot-config-validation branch July 15, 2021 16:51
samarsault pushed a commit that referenced this pull request Mar 3, 2023
Bumps [cypress](https://github.com/cypress-io/cypress) from 8.4.1 to 8.5.0.
- [Release notes](https://github.com/cypress-io/cypress/releases)
- [Changelog](https://github.com/cypress-io/cypress/blob/develop/.releaserc.base.js)
- [Commits](cypress-io/cypress@v8.4.1...v8.5.0)

---
updated-dependencies:
- dependency-name: cypress
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

✨ enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Validate specific config options

2 participants