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

Storyshots: Fix typings of "test"-method #12389

Merged
merged 2 commits into from Oct 5, 2020
Merged

Conversation

nknapp
Copy link
Contributor

@nknapp nknapp commented Sep 4, 2020

Issue: #12361

What I did

Adjust the typescript definition of storyshot's "test" method to better reflect reality.
It could be improved, but I am not sure which args are really passed all the time
(or maybe only depending on the framework).

How to test

  • Is this testable with Jest or Chromatic screenshots? no
  • Does this need a new example in the kitchen sink apps? no
  • Does this need an update to the documentation? maybe

If your answer is yes to any of these, please make sure to include it in your PR.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Thanks for digging into this! I'm not too familiar with this code, but it seems like more changes are needed, e.g.

https://github.com/storybookjs/storybook/blob/next/addons/storyshots/storyshots-core/src/test-bodies.ts#L15-L20

Any chance you can do a more thorough audit of the code to tighten things up as part of this change?

@nknapp
Copy link
Contributor Author

nknapp commented Sep 5, 2020

Let's see how it works now. I have done the following changes:

  • fix the TestMethodOptions based on the types found in test-bodies (use a union of all arg-types of all methods defined there).
  • refactor the test-bodies to use the new StoryshotsTestMethod. I was a bit confused at first, about what the functions are doing, and I hope that my change makes it clear that they are actually returning test-methods.

Both changes are in separate commits, you can pick either

@nknapp
Copy link
Contributor Author

nknapp commented Sep 8, 2020

@shilman I have made some changes and they are ready for review. I am not sure if I have to do anything to change the state of this PR so that it gets your attention again. It is still in "Changes requested" state.

@shilman
Copy link
Member

shilman commented Sep 8, 2020

@nknapp i'm on it 😄

@stale stale bot added the inactive label Oct 4, 2020
@storybookjs storybookjs deleted a comment from stale bot Oct 5, 2020
@stale stale bot removed the inactive label Oct 5, 2020
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

LGTM!

@shilman shilman changed the title fix(storyshots): match typings of "test"-method to real usage Storyshots: Fix typings of "test"-method Oct 5, 2020
@shilman shilman merged commit 368bde8 into storybookjs:next Oct 5, 2020
@shilman shilman added this to the 6.1 maintenance milestone Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants