Skip to content

Commit

Permalink
Change e2e snapshot to use allowSharedSnapshot
Browse files Browse the repository at this point in the history
  • Loading branch information
@flotwig
flotwig committed Oct 16, 2019
1 parent 95b3af7 commit c3aaf97
Showing 1 changed file with 3 additions and 10 deletions.
13 changes: 3 additions & 10 deletions packages/server/test/support/helpers/e2e.coffee
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@ normalizeStdout = (str, options = {}) ->
.replace(/(Uploading Results.*?\n\n)((.*-.*[\s\S\r]){2,}?)(\n\n)/g, replaceUploadingResults) ## replaces multiple lines of uploading results (since order not guaranteed)
.replace(/^(\- )(\/.*\/packages\/server\/)(.*)$/gm, "$1$3") ## fix "Require stacks" for CI

str = str.replace(/\(\d{2,4}x\d{2,4}\)/g, "(YYYYxZZZZ)") ## screenshot dimensions
if not options.keepScreenshotDimensions

This comment has been minimized.

Sign in to view

Copy link
@brian-mann

brian-mann Oct 16, 2019 •
edited
Loading

Member

These kinds of options should be by default given in the affirmative in the typical case. The default here is to do something (to remove the screenshot dimensions) and therefore the default value is true.

The OVERRIDE is the opt-in, where what you're opting into takes away a behavior.

if options.sanitizeScreenshotDimensions
  ## do the thing

e2e.it "passes", {
  spec: "screenshot_viewport_capture_spec.coffee"
  expectedExitCode: 0
  snapshot: true
  santizeScreenshotDimensions: false
}

Since the default option is to always remove the screenshot dimensions, then the override only makes sense to turn this behavior off, which makes more sense as a false value.

This comment has been minimized.

Sign in to view

Copy link
@flotwig

flotwig Oct 16, 2019

Author Contributor

ab00ecd

This comment has been minimized.

Sign in to view

Copy link
@jennifer-shehane

jennifer-shehane Oct 16, 2019

Member

This replacement algorithm is too naive. This will replace (15x15) with (YYYYxZZZZ) and (150x4000) with YYYYxZZZZ), throwing off the table calculations in the stdout PR. Needs padding calculated.
str = str.replace(/\(\d{2,4}x\d{2,4}\)/g, "(YYYYxZZZZ)") ## screenshot dimensions

return str.split("\n")
.map(replaceStackTraceLines)
Expand Down Expand Up @@ -413,15 +414,7 @@ module.exports = e2e = {

str = normalizeStdout(stdout, options)

args = _.compact([options.originalTitle, str])

try
snapshot(args...)
catch err
## if this error is from a duplicated snapshot key then
## ignore it else throw
if not err.duplicateSnapshotKey
throw err
snapshot(options.originalTitle, str, { allowSharedSnapshot: true })

return {
code: code
Expand Down

0 comments on commit c3aaf97

Please sign in to comment.