garris / ember-backstop

BackstopJS visual regression testing addon for Ember.
MIT License
56 stars 21 forks source link

Use Environment variable(ENABLE_BACKSTOP) to enable/disable backstop assertions #37

Closed shankarsridhar closed 4 years ago

shankarsridhar commented 4 years ago

Backstop assertions should not be run by default, since it may have unintended negative consequences like failing tests(first time tests run without established reference screenshots).

This could be a serious problem, particularly when backstop assertions are run as part of tests suite in CI/CD pipeline.

This PR is to flip the default behaviour from backstop assertions running by default to not running by default. To opt-in to run backstop assertions(take screenshots), please refer to the steps in README.

shankarsridhar commented 4 years ago

@garris I have verified this change against ember-backstop-tutorial and some other emberish apps which have a slightly different folder structure.

shankarsridhar commented 4 years ago

@garris Please let me know if my understanding of the code review comments is correct:

garris commented 4 years ago

Hi @shankarsridhar --

1) yes, in public ember-backstop, the kill switch is "opt-in".

2) Not exactly. A) In the Pemberly implementation-- we want to require end-users to to ENABLE_BACKSTOP explicitly when they run tests. This is a guardrail -- EG engineers who are used to our existing test infra who call ember test won't be surprised by this new heavy visual test process. So, passing ENABLE_BACKSTOP = true at the command line makes sense. But B) other public users may not need/want this behavior. It is great to include the readme documentation to show how env cars can be used -- but I don't expect every implementation would want to do it this way.

IOW: what we are saying with disableBackstop is: here is a kill switch -- just pull this thing anytime you need to disable these heavy visual tests -- but how you expose this to end users (engineers) is up to you. _Pemberly will choose to expose this to end-users as an "ENABLEBACKSTOP" flag.

Please let me know if this isn't coming through clearly. The ergonomics at play here are pretty subtle.

garris commented 4 years ago

Pushed to NPM 👍

npm notice === Tarball Details ===
npm notice name:          ember-backstop
npm notice version:       1.4.0
npm notice package size:  897.1 kB
npm notice unpacked size: 1.7 MB
npm notice shasum:        9f09371a0fffc23b472c59cbd96b5193ff12ad41
npm notice integrity:     sha512-9caaJy3J8Tso1[...]ANiZOglunQBdg==
npm notice total files:   83
npm notice
+ ember-backstop@1.4.0