Refactor Visual Tests stories to more closely control selected story

[tmeasday]

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 (#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.

• We have a bunch of stories that hard code the returned lastBuildOnBranch and selectedBuild.
• However, the VisualTests component changes the selectedBuildId variable based on various criteria.
• This was not reflected in the stories as the selectedBuild was hard-coded.

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:

addon-visual-tests/src/screens/VisualTests/VisualTests.tsx

Lines 158 to 163 in 0030d1e

	// Before we set the storyInfo, we use the lastBuildOnBranch for story data if it's ready
	const selectedBuild = getFragment(
	FragmentSelectedBuildFields,
	data?.selectedBuild ??
	(lastBuildOnBranchCompletedStory ? data?.project?.lastBuildOnBranch : undefined)
	);

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

✨ Test out this PR locally via:

npm install @chromaui/[email protected]
# or 
yarn add @chromaui/[email protected]

[linear]

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

[weeksling]

Sorry for delayed review. This story makes sense to me. I'm not sure about changing every story to have a function but if the change could be handled in the withBuilds method then it makes sense.

Now that I understand them a bit better, I think a high level mock is useful. For edge cases and interactions presentational stories might work well but I think this issue for instance might have been hard to test without a graphql mock that mimics our api even if it required changing.

The story looks good to me. I can comment on the other questions mentioned tomorrow after a night's rest.

[ghengeveld]

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]

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.

[tmeasday]

As noted this play function is disabled here (and in 2 other tests) but fixed in #130

[ghengeveld]

Nice, I like this approach 👍