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

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

Open
moellering opened this issue Jan 31, 2018 · 4 comments

Comments

@moellering
Copy link

moellering commented Jan 31, 2018

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)

@jlane9
Copy link
Owner

jlane9 commented Jan 31, 2018

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
Copy link
Owner

jlane9 commented Jan 31, 2018

@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

@moellering
Copy link
Author

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 moellering changed the title Suggestion: Simplify adding assert_screenshot to existing test cases Derive screnshot path from details like viewport size, browser and/or a label Feb 1, 2018
@jlane9
Copy link
Owner

jlane9 commented Feb 1, 2018

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.

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

No branches or pull requests

2 participants