pytest-needle icon indicating copy to clipboard operation
pytest-needle copied to clipboard

Derive screnshot path from details like viewport size, browser and/or a label

Open moellering opened this issue 7 years ago • 4 comments

Feature Suggestion

Originally this was a feature suggestion which is continued in issue #10 (The original content of this issue can be found there)

Now this issue covers proposals by @jlane9 a. Save baselines for different screen sizes b. Save baselines for different browsers c. Save incremental baselines for each successful run, compare against most recent baseline set (Sort of like your 2nd suggestion)

moellering avatar Jan 31 '18 11:01 moellering

From what I can gather, these are your suggestions:

  1. Allow runs without baselines to create their own baselines (possibly with optional needle-capture flag)
  2. Allow successful runs to generate "fresh" baseline images

There were a few other similar cases that I have thought of previously:

a. Save baselines for different screen sizes b. Save baselines for different browsers c. Save incremental baselines for each successful run, compare against most recent baseline set (Sort of like your 2nd suggestion)

Personally, I would prefer your second suggestion too. There's probably a good reason needle deprecated the first suggestion in their project.

jlane9 avatar Jan 31 '18 20:01 jlane9

@moellering I started a branch, I've implemented my suggestions a. , b. & c. and I am now moving to your second suggestion.

Branch: https://github.com/jlane9/pytest-needle/tree/issue_9

jlane9 avatar Jan 31 '18 22:01 jlane9

Thank you for taking your time and thinking of ways to improve the scripts.

And I like the possiblity of distinguishing between runs. However what you are currently implementing is backwards incompatible and will screw up ppl who have written scripts around their testsuite.

Remarks: a) This is a absolutely valid use case since most of the time the baseline images will not be valid anymore, once someone runs the tests with a different screen size. Unfortunately there is a use case which will break if we force users to add the viewport-size: Comparing one specific element on different screen sizes. If you want to add an option append_viewport_size to assert_screenshot (default false) and or a command line option --needle-append-viewport-size which appends the viewport size to a filename. This way we won't break backwards compatibility while still offering this option. b) Usually I expect websites to look on different browser exactly the same. Enabling putting baseline images into browser specific folders would prevent me from checking this. If you really want this feature you should put it into a command line option. More remarks see below. c) This one is option so i like it. But I don't think it is really necessary.

However all of the above (especially b) and c) ) can be done already. When anyone runs pytest regularly with different parameters (browser, screensize etc) this call will be in a script. By using pytest --needle-output-dir=/screenshots/$DRIVER/$SIZE/$BUILD --driver $DRIVER ----needle-viewport-size "$SIZE" I can do basically the same as you propose to implement in your commit.

If you want to add this functionality, that's fine, but changing default behavior will break peoples testing setups. That would be a big Nope for me. Maybe you can change the defaults later on when you to do major version step, but I'd propose to communicate that very clearly and early on.

To keep things a little more tidy I created a new regarding my initial proposal: #10

moellering avatar Feb 01 '18 11:02 moellering

Thanks for the feedback @moellering

I've went ahead and made this mode optional (you have to specify using, rather than it being the default). I could someday make each toggleable but for now is all on or off with flag --needle-categorize

For you remarks:

a) viewport-size is already defaulted to 1024x768, so viewport-size is not necessary b) certain elements in different browsers are styled slightly differently. Selects are probably the most noticeably different. Since this is optional you still should be able to run through your use case

I'll leave the auto creating baselines to #10.

jlane9 avatar Feb 01 '18 19:02 jlane9