Need to make the storybook addon-storyshots more stable

[brentswisher]

In working to add all of the components to Storybook, addon-storyshots is proving difficult to maintain. The biggest issue in my mind is that all of the snapshots run in the same context, causing two problems:

1. It generates a single snapshot (currently 16,874 lines long) with all of the components which is difficult to understand when something changes
2. Any component that uses useInstanceId as its id (e.g. SelectControl ) gets incremented whenever another story with that component gets added. This causes confusion as un-related stories snapshots can change whenever a new one is added.

There is an option in the add-on to split the snapshots into individual files located in the /stories/ directory within each component. Unfortunately, this doesn't prevent the second issue because the stories are still executed in the same context, the results are just written out to different files. It does, however, make it easier to see what happened.

I have a proof-of-concept PR started to show how the individual snapshots might look: #21484

Based on my research into the add-on, I can't see how to get around the first problem without writing separate snapshot tests for each story. That seems to defeat the whole purpose of the addon - at that point, we are just rewriting the unit-tests of the component in my opinion.

So, the other option is to stop using addon-storyshots altogether. I thought I would open this issue to serve as a place for discussion. I'm happy to do the work, I just was hoping to get some opinions on which way to go or if there were any other options I was missing.

[youknowriad]

So, the other option is to stop using addon-storyshots altogether

I'd argue that this is the best move too. I experienced these difficulties myself in several PRs.

One potential good thing about story shots was the automatic a11y tests but these are not implemented yet. So I'd be in favor of removing the test for now until we figure out better options. Right now, these tests are useless.

[brentswisher]

Any other thoughts @gziolo @ItsJonQ @aduth? After struggling with issues again this past week I am leaning toward removing it. Right now it is making it considerably harder to add stories. Unless there is someone strong dissent, I will work on a PR removing it this week.

[ItsJonQ]

So, the other option is to stop using addon-storyshots altogether

I'd agree with this as well 👍. They've been tricky for me to work with 😅

I will work on a PR removing it this week.

That would be awesome! Thank you!

[aduth]

I'm not opposed to removing it, and for me it's probably the best option for the moment.

That being said, to the noted issues:

1. "difficult to understand when something changes"
   • While I don't think difficult is the goal here, it's very much the goal of snapshots as forcing the developer to make a conscious choice to "approve" a change as being intentional.
2. "Any component that uses useInstanceId as its id (e.g. SelectControl ) gets incremented"
   • Yes, this is the most troublesome, since it's highly unstable. There is some prior art for addressing this sort of thing, notably the block fixtures testing has a similar problem, where clientIds are random, but for the purpose of the fixtures are changed to a predictable string value:

     • gutenberg/packages/e2e-tests/fixtures/blocks/core__rss.json

       Line 3 in 4924eff

       "clientId": "_clientId_0",

[brentswisher]

"1. difficult to understand when something changes"

Yes, I should have been clearer. The combination of the useInstanceId issue and it all being one large file makes it difficult to see what is actually changing when a snapshot fails, making it hard to make that "approve" decision.