chromaui / addon-visual-tests

Visual Tests addon for Storybook
MIT License
33 stars 2 forks source link

Refactor Visual Tests stories to more closely control selected story #124

Closed tmeasday closed 11 months ago

tmeasday commented 11 months ago

This PR does 2 main things:

  1. Refactor VisualTests so the selectedBuildId is a prop. This allows us to continue to write stories that vary the selectedBuild + lastBuildOnBranch without having weird stuff happen with the selectedBuildId. It also allows us to clearly assert on when the component should change the selectedBuildId in play functions[1].

  2. Refactor the stories to use an args based approach and be very careful about the builds we pass. I'm pretty happy with how the stories end up reading but open to feedback.

[1] This also exposed a bug in how we are doing it right now that I'll fix in a follow up PR (https://github.com/chromaui/addon-visual-tests/pull/130)


Old text:

So the behaviour was actually right here, but the story didn't show it.

Let me explain because I think we might want to consider tightening up our testing strategy here.

For the older story Empty Branch Local Build Captured Current Story we fudge around this, because we set the selectedBuild to the lastBuildOnBranch when there is no selectedBuild:

https://github.com/chromaui/addon-visual-tests/blob/0030d1ef81fd19e57566fe83551d96c5a1648c09/src/screens/VisualTests/VisualTests.tsx#L158-L163

However, that doesn't help us for the newer, similar story Story Added In Last Build On Branch Not In Selected because the selected build does exist, it just doesn't have this story.

So the fix here is to make the GraphQL mock actually respond to the change in selectedBuildId. My question for y'all is: Should we, to be correct, make that change to all these stories? Or is this whole approach of testing the VisualTests component wrong and we should just write stories at the presentational level and do some other type of testing of "what happens when the selectedStoryId changes?" or "what happens when there's a new lastBuildOnBranch?"

Possibly we drop the quoted lines above too as it's sort of confusing there are two ways the selectedBuild ends up being equal to the lastBuildOnBranch. (Firstly by the short cut above, secondly by setting selectedBuildId, requerying, and re-rendering). Then again the shortcut saves the user waiting for a second query.

Another option would be to extend the logic to make selectedBuild = lastBuildOnBranch if the current story isn't in the actual selected build. Confusing?

📦 Published PR as canary version: 0.0.106--canary.124.6cd88ba.0
:sparkles: Test out this PR locally via: ```bash npm install @chromaui/addon-visual-tests@0.0.106--canary.124.6cd88ba.0 # or yarn add @chromaui/addon-visual-tests@0.0.106--canary.124.6cd88ba.0 ```
linear[bot] commented 11 months ago
AP-3703 Auto switch to latest if story is on latest build, but not on selected build

Currently, if a user is on a story that exists on the last build on the branch, but not on the locally selected build, it will still display "new story ui". To refresh it, they need to change out of the story and back. Instead, if the story is on the latest selected build, we should automatically change to the latest build. [https://www.chromatic.com/review?appId=6480e1b0042842f149cfd74c&number=101&activeElementId=comment-thread-650e6df037c742aeeaf18086](https://www.chromatic.com/review?appId=6480e1b0042842f149cfd74c&number=101&activeElementId=comment-thread-650e6df037c742aeeaf18086)

ghengeveld commented 11 months ago

So the fix here is to make the GraphQL mock actually respond to the change in selectedBuildId. My question for y'all is: Should we, to be correct, make that change to all these stories? Or is this whole approach of testing the VisualTests component wrong and we should just write stories at the presentational level and do some other type of testing of "what happens when the selectedStoryId changes?" or "what happens when there's a new lastBuildOnBranch?"

I prefer the "integration test" style of stories over the pure presentational level, because it showcases what Storybook can do rather than copping out, while also providing us with better coverage. So I think the approach taken here is desirable. "some other type of testing" would likely mean E2E which I'd like to avoid if possible.

tmeasday commented 11 months ago

Ok, so it sounds like the consensus here is to apply this approach to the other stories in the VisualTests component. I'll take another pass at it.