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

Update/compare mode/doc #1

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

AshCoolman
Copy link
Owner

Compare pages API

📖 📖 docs only 📖 📖 , implementation to follow

Background

Work done

I have a proof of concept working on my local machine, so now I'm thinking about a good API.

Attempt 1 (commit)

Uses config objects and introduces an internal cache object to keep store image data for later comparisons. This roughly matches the different parameter usage that toMatchSnapshot has when chained vs unchained.

Attempt 2 (current state)

I then decided against adding internal state. If that is done for every new feature, the lib might not scale so well? I though perhaps better to expose (the edges of) the internals, and give people the power.

So, I propose:

  1. exposing the internal state via "introspect". It takes a function which it calls with a proxy (lowercase "p") object with certain exposed state - aka a big bag of variables.
  2. adding a compareImages function, with a config object

Alternative

If you don't like these two, initially I was passing in the spec, and using that to access the appropriate snapshot to compare against.

var spec = it('test', () => await.browser.goto(LIVE).screenshot().toMatchSnapshot())
it('live', () => await.browser.goto(LIVE).screenshot().toMatchSnapshot().compareWithSpec(spec)

Notes

Due to the async control-flow of the chained API, the cached image that is passed to .compareImages needs to be accessed dynamically.

@AshCoolman AshCoolman added the help wanted Extra attention is needed label Jan 26, 2018
@AshCoolman AshCoolman self-assigned this Jan 26, 2018
@NimaSoroush
Copy link
Collaborator

Thanks @AshCoolman for putting these together,

Attempts1:
Regarding Attempt 1: I just added comment on your commit : 9435436

Attempt2:
Attempt2 looks fine but it needs more work and refactoring which might not be neccessary I guess! we might want to have proper state managment solution in this case

Alternative:
I think either of 1 or 2 would be better solution. 3rd option looks more confusing to the user

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
2 participants