feat: Enable TurboSnap ⚡ 🤑 (I don't know if this will work.) #4768
Conversation
AshCorr
requested review from
a team,
JamieB-gu,
marsavar and
iainjchambers-guardian
as code owners
|
Size Change: 0 B Total Size: 1.63 MB ℹ️ View Unchanged
|
|
I'm a bit cautious with this change. My feeling is we probably want to lean more towards spending extra money and having better test coverage/dev ex vs. reduce coverage and saving money. How much is test coverage worth? It's hard to quantify but I think that it's true that the benefit we gain from having such a wide suite of regression tests would not be possible to replace with one QA engineer. In theory if this feature delivers on what it offers then we get the best of both worlds but I think we should only add the complexity and risk if we actually think we have a problem here. Maybe the extra cost we're spending is worth it and the business would prefer to have the increased confidence? |
Ideally we wouldn't have any reduction in test coverage, but I don't really know enough about how this works to know for sure that it won't reduce coverage. From my perspective, I'm hesitant to add things like breakpoints to any stories I create because I know that by doing that I'm adding a not insignificant cost to every commit for the foreseable future of DCR. Even if its a cost the business is willing to accept I would still personally hesitate to do it because I'm a bit of a scrooge. If I knew that something wasn't going to add any extra cost I'd quite happily add breakpoints to every component and enable cross browser testing. |
|
I find this an incredibly interesting idea, but I also have quite a lot of hesitancy towards it. First of all I think it's important that as developers we are not worrying about 'cost per commit', or the cost of PRs at all. Commits, PRs, CI, testing, these are all fundamentals of our job and it's the businesses responsibility to provide us with the funds required to operate properly. I think from that it's important to recognise the value that Chromatic adds to our work as developers. In my opinion the value is unquantifiable, but it's safe to say that in terms of our output as a team, it's worth at least 2-3 additional developers in the safety and confidence it gives us to work quickly and make changes ~ the starting salary for a mid level developer is £49,600. Ideological stuff out of the way, my technical fear of this change is that we will end up missing changes, especially since our webpack project is quite a complex mix between SSR and client rendering. The only way to find out how well it works of course would be to test it! The best way to do this would be to open some test PRs based off of this branch, and make some changes in key areas of the site ~ layouts, rendering components, decidePalette, etc. The above would give us a solid idea of whether this works at all - essential to us considering this as a workable solution for DCR! If successful, I'd recommend that we then spend some time considering the risk of edge cases, if they can be accounted for another way (such as running a full chromatic check once a PR has been approved, but not on earlier commits?). |
Hold on, which webpack config does it use? I was assuming it would use Storybook's webpack rather than attempt to understand all the different ways a project's own webpack could be set up? The one configured in |
I would imagine this one too - but this doesn't really help my discomfort, I don't know storybook bundles or interprets islands for example, we'd want to test if it would pick up a change to an island the way we'd expect! The main.js webpack config is a modified version of storybooks config, which also means its hard for us to get a clear picture of exactly what its doing under the hood, and it also means it could change in ways we might not anticipate |
|
Worth trialing this for a day or two to see how it reacts to our codebase? |
I'd prefer if we trialled it in specific use cases by opening PRs with the feature enabled in the github action config for that branch, and looking at the output compared to having it disabled, as this will give us much more explicit data on how it works! |
I can try doing this, I think GHA only runs on the version of the workflow thats on the main branch, not the one thats on the branch. Might be able to setup a condition in the main workflow to change the behaviour based on what branch its on. (or see if i can get hold of a chromatic key to run all this locally...) |
Ah I didn't know this, I assumed it would always use the one in the current branch! My bad sorry Could you create a new chromatic workflow that targets a specific branch or branches you can use for testing? e.g # chromatic-turbo.yaml
on:
push:
branches:
- chromatic-test-1
- chromatic-test-2
... chromatic stuff ... |
Thats a much better idea than what I came up with, I'll give it a go tomorrow! |
|
Update: This may or not be relevant now, having just seen the last couple of messages in the thread, but I'll leave it up in case it is of use: I had also thought that GHA would only use the workflow from the main branch, but our experience testing out #4773 today suggests that it does actually run workflow from the branch being PRed. (Are you able to confirm that @mxdvl ?) |
EDIT: Apparently theres an "Approval" required for Forks to run their version of a workflow, but branches in the main repo itself are apparently not restricted at all. |
|
Tested this with a couple of cases, including islands... It seemed to be able to identify storybook files that depend on changed components but had issues identifying Stories generated using Closing this until storybookjs/storybook#9828 is resolved, hopefully that will give us an option to use dynamically generated CSF |
Update
Reviving as #4891 didn't make any significant dents. This does still mean we can't squash merge, but hopefully as a department, we might be able to feedback to the Chromatic team that this does break workflows and help find a solution.
This can run as a test for a ~week to help us assess whether we have confidence that it does alert us to all changes, and what savings it might make.
This has been recommended by our friends at Chromatic.
What does this change?
Enables TurboSnap https://www.chromatic.com/docs/turbosnap on Chromatic builds.
How?
TurboSnap looks at the Webpack dependency graph to figure out what components have changed between two commits and then uses that information to restrict snapshots to components that have changed.
Why?
It's starting to get fairly costly to run Chromatic tests on every single commit, some quick napkin maths would suggest that we use ~10USD per commit. I think we're all agreed that Chromatic is an amazing tool and that we should keep using it, but we should try and look at ways we can reduce its cost.
Potential Issues
Pull requests
According to the Chromatic docs TurboSnap isn't exactly compatible with PR's due to how it works out changes - BUT, I think this incompatibility won't actually cause us too many issues... As I understand the problem is that Chromatic will ignore approvals on any previous commits and re-request approvals on changes that have already been approved (earlier commits in the PR)
It might be a little annoying to re-approve changes, but maybe its worth it for the cost save?
Sqush & Merge
The "Squash & Merge" merge option that Github provides might cause issues.