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

explain how to make async tests work #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shlomitc
Copy link
Contributor

No description provided.

Change your `applitools.config.js` file to the following:
```js
module.exports = {
waitBeforeScreenshots: `[data-test-ready="true"]`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I actually created a dataHooks file for this.
I thought that users could import the datahook from the file, and use it

const { DATA_READY_HOOK } = require('storybook-snapper/dist/src/dataHooks');
module.exports = {
  waitBeforeScreenshots: `[${DATA_READY_HOOK}="true"]`,
  ...
}

wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this, it's internal implementation and import is ugly. We should provide default configuration with some way to override it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what you're saying is that you want to have eyes-storybook as an internal implementation of this library?
And maybe add a binary that will run eyes-storybook?
And users will be able to add a config file with overrides?

So all configuration will be done in one place?

Hmmm 🤔
I think I like the idea.
So no one will really need to deal with eyes-storybook themselves, and in the future, we might be able to change applitools for something else if we want.

wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is the case, then we might need to change the name to something without storybook (although if we ever use something else than storybook, consumers will have their tests be broken)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrapping eyes-storybook sounds a bit too much at the moment. I actually thought exposing the storybook config instead like storybook's webpack config extension works.

const {storybookSnapperApplitoolsConfig} = require('storybook-snapper');
module.exports = storybookSnapperApplitoolsConfig({
  //rest of the configuration
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.
I usually don't like having multiple "moving parts", but I guess as a start it's a good way to go

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, what I initially wrote is also "moving parts", so I guess your approach will be better for now

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.

None yet

2 participants