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

Feat/1432 global scenario settings #1539

Merged
merged 3 commits into from
Feb 1, 2024

Conversation

dgrebb
Copy link
Contributor

@dgrebb dgrebb commented Jan 23, 2024

Problem

As described in #1432 and other issues, setting/getting scenario properties at the global level would save time, typing, and testing overhead.

Solution

Spread (...) and logical OR (||) operator.

Important

This feature is opt-in only, and documented as such

Because configuration at the scenario level will always override any globals set in backstop.json, a user will need to remove the corresponding property in a scenario object.

This is a 100% non-breaking change, thanks to spread...

Supported Global Properties

  • cookiePath
  • url - maybe we have a single-page app with no routes?
  • readySelector
  • delay
  • hideSelectors
  • removeSelectors
  • hoverSelector
  • clickSelector
  • postInteractionWait
  • selectors
  • selectorExpansion
  • misMatchThreshold - previously supported but refactored getMisMatchThreshHold
  • requireSameDimensions - previously supported but refactored getRequireSameDimensions

Added to both Playwright and Puppeteer.

Closes

Notes

An example:

Expand `backstop.json`
{
  "id": "backstop_playwright",
  "viewports": [
    {
      "label": "phone",
      "width": 320,
      "height": 480
    },
    {
      "label": "tablet",
      "width": 1024,
      "height": 768
    }
  ],
  "onBeforeScript": "playwright/onBefore.js",
  "onReadyScript": "playwright/onReady.js",
  "globals": {
    "cookiePath": "backstop_data/engine_scripts/cookies.json",
    "url": "https://garris.github.io/BackstopJS/",
    "readySelector": "",
    "delay": 0,
    "hideSelectors": [".getItBlock"],
    "removeSelectors": [".logoBlock"],
    "hoverSelector": "",
    "clickSelector": "",
    "postInteractionWait": 1000,
    "selectors": [],
    "selectorExpansion": true,
    "misMatchThreshold" : 0.1,
    "requireSameDimensions": true
  },
  "scenarios": [
    {
      "label": "BackstopJS Homepage",
      "cookiePath": "backstop_data/engine_scripts/cookies.json",
      "url": "https://garris.github.io/BackstopJS/",
      "referenceUrl": "",
      "readyEvent": "",
      "readySelector": "",
      "delay": 0,
      "hoverSelector": "",
      "clickSelector": "",
      "selectors": [],
      "selectorExpansion": true,
      "misMatchThreshold" : 0.1,
      "requireSameDimensions": true
    }
  ],
  "paths": {
    "bitmaps_reference": "backstop_data/bitmaps_reference",
    "bitmaps_test": "backstop_data/bitmaps_test",
    "engine_scripts": "backstop_data/engine_scripts",
    "html_report": "backstop_data/html_report",
    "ci_report": "backstop_data/ci_report"
  },
  "report": ["browser"],
  "engine": "playwright",
  "engineOptions": {
    "args": ["--no-sandbox"]
  },
  "asyncCaptureLimit": 5,
  "asyncCompareLimit": 50,
  "debug": false,
  "debugWindow": false,
  "archiveReport": true,
  "scenarioLogsInReports": true
}

image

Next Steps

We could discuss moving the global onBefore and onReady scripts into the new globals object, but it would be a breaking change, or duplicative (to classify as non-breaking), as we'd have them repeated as such:

  "onBeforeScript": "puppet/onBefore.js",
  "onReadyScript": "puppet/onReady.js",
  "globals": {
    "cookiePath": "backstop_data/engine_scripts/cookies.json",
    "url": "https://garris.github.io/BackstopJS/",
    "onBeforeScript": "puppet/onBefore.js",
    "onReadyScript": "puppet/onReady.js",

@JPustkuchen
Copy link

Great work @dgrebb. 🥳
What do you think about renaming "globals" to "defaults" (my fav) or "scenarios-defaults"?
I think 'globals' might not be perfect wording? Any better ideas?

@dgrebb
Copy link
Contributor Author

dgrebb commented Jan 23, 2024

Thanks @JPustkuchen!

For maintainability, especially because the project has (and will have more!) numerous contributors, I would suggest avoiding the word "default" entirely.

There are some default patterns baked in to the core browser runners which use this naming convention:

  • DEFAULT_FILENAME_TEMPLATE
  • DEFAULT_BITMAPS_TEST_DIR
  • DEFAULT_BITMAPS_REFERENCE_DIR
  • engine_scripts_default
  • setDefaultNavigationTimeout
  • etc...

These are used when running backstop init, and provide values in the generated, default backstop.json.

To avoid breaking changes, this new "globals" object (or whatever we call it) may not be included in the backstop.json one gets out of the box.

I'm not in love with the name, either. We have a tough job. Perhaps simply using a top level "scenario" (singular) would save us the torment of going down a naming rabbit hole, but I digress.

Will think on this, and also give time for others to discover the party we have going on in here 🎉

Thank you for the suggestions!

@dgrebb dgrebb closed this Jan 29, 2024
@dgrebb dgrebb deleted the feat/1432-global-scenario-settings branch January 29, 2024 22:44
@dgrebb
Copy link
Contributor Author

dgrebb commented Jan 29, 2024

@garris as discussed I've renamed the global scenario configuration object scenarioDefaults. This is ready for a merge.

@JPustkuchen FYA — you almost got what you asked for :)

@dgrebb dgrebb reopened this Jan 29, 2024
@JPustkuchen
Copy link

GREAT WORK, thank you @dgrebb !! 🎉

@garris garris merged commit 0c8ead0 into garris:master Feb 1, 2024
4 checks passed
@garris
Copy link
Owner

garris commented Feb 1, 2024

Thank you @dgrebb! ⭐️

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