git is required for visual test addon via chromatic cli #126

weeksling · 2023-10-03T23:30:39Z

This adds an error state to the panel, disables the run button, and shows a notification if the addon is run in a project without git configured.

To test this:

Run storybook with addon installed.
Rename or delete .git directory to trigger an error.
It should show a single notification and update the panel with the git error.

Note: this currently stops the gitObservable loop from occurring and requires the server to be restarted after fixing the issue. Added language in the notification to prompt the user to restart storybook after fixing.

Note: This does not handle build errors once the gitInfo can be retrieved. The pr this branches off of should handle build-level errors including the git one commit issue.

📦 Published PR as canary version: 0.0.99--canary.126.8f69032.0

✨ Test out this PR locally via:

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

linear · 2023-10-03T23:30:49Z

AP-3618 Git is required for visual test addon via chromatic CLI

Action Item: handle git errors from the CLI and display this design in the addon:

Panel - Visual testing in Storybook (Figma)

Feedback:

The addon uses Chromatic under the hood which requires a Git remote with at least one commit to work. The main issue with this is that the error isn't handled with proper help and since I don't necessarily know the Chromatic CLI is being used under the hood, I wouldn't know where to go for help.

At the least, the error should be handled with instructions but ideally a Git commit isn't required for the visual test addon to work (not sure whether this is feasible).

Reproduction

Init SB with Vite + React TS in an empty dir
Add the visual test addon

Start Storybook ← This errors with:

info => Starting manager..
Unhandled promise rejection: Error: Command failed with exit code 1: git config --get remote.origin.url
    at Eir (/Users/zol/Git/test-visual-testing-addon/node_modules/chromatic/dist/chunk-J3MH667B.js:197:57)
    at /Users/zol/Git/test-visual-testing-addon/node_modules/chromatic/dist/chunk-J3MH667B.js:197:8411
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async Ki (/Users/zol/Git/test-visual-testing-addon/node_modules/chromatic/dist/chunk-J3MH667B.js:287:45)
    at async NM (/Users/zol/Git/test-visual-testing-addon/node_modules/chromatic/dist/chunk-J3MH667B.js:287:530)
    at async Object.N3i (/Users/zol/Git/test-visual-testing-addon/node_modules/chromatic/dist/chunk-J3MH667B.js:1298:1359)
    at async c (/Users/zol/Git/test-visual-testing-addon/node_modules/chromaui/addon-visual-tests/dist/index.js:12:1250) {
  shortMessage: 'Command failed with exit code 1: git config --get remote.origin.url',
  command: 'git config --get remote.origin.url',
  escapedCommand: 'git config --get remote.origin.url',
  exitCode: 1,
  signal: undefined,
  signalDescription: undefined,
  stdout: '',
  stderr: '',
  all: '',
  failed: true,
  timedOut: false,
  isCanceled: false,
  killed: false
}
Error: Command failed with exit code 1: git config --get remote.origin.url
    at Eir (./node_modules/chromatic/dist/chunk-J3MH667B.js:197:57)
    at ./node_modules/chromatic/dist/chunk-J3MH667B.js:197:8411
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async Ki (./node_modules/chromatic/dist/chunk-J3MH667B.js:287:45)
    at async NM (./node_modules/chromatic/dist/chunk-J3MH667B.js:287:530)
    at async Object.N3i (./node_modules/chromatic/dist/chunk-J3MH667B.js:1298:1359)
    at async Object.env (./node_modules/chromaui/addon-visual-tests/dist/index.js:14:290)
    at async Promise.all (index 3)
    at async getConfig (./node_modules/storybook/builder-manager/dist/index.js:1:5670)
    at async Promise.all (index 1)

Creating a repo on Github and running git remote add origin gitgithub.jparrowsec.cn:zol/test-visual-testing-addon.git Gets me past the above but now I hit:

info => Starting manager..
Unhandled promise rejection: Error: ✖ Unable to execute command: git --no-pager log -n 1 --format="%H ## %ct ## %ae ## %an" 
Chromatic requires your Git repository to have at least one commit.
    at Ki (/Users/zol/Git/test-visual-testing-addon/node_modules/chromatic/dist/chunk-J3MH667B.js:287:334)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async cm (/Users/zol/Git/test-visual-testing-addon/node_modules/chromatic/dist/chunk-J3MH667B.js:287:677)
    at async Object.N3i (/Users/zol/Git/test-visual-testing-addon/node_modules/chromatic/dist/chunk-J3MH667B.js:1298:1385)
    at async c (/Users/zol/Git/test-visual-testing-addon/node_modules/chromaui/addon-visual-tests/dist/index.js:12:1250)
Error: ✖ Unable to execute command: git --no-pager log -n 1 --format="%H ## %ct ## %ae ## %an" 
Chromatic requires your Git repository to have at least one commit.
    at Ki (./node_modules/chromatic/dist/chunk-J3MH667B.js:287:334)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async cm (./node_modules/chromatic/dist/chunk-J3MH667B.js:287:677)
    at async Object.N3i (./node_modules/chromatic/dist/chunk-J3MH667B.js:1298:1385)
    at async Object.env (./node_modules/chromaui/addon-visual-tests/dist/index.js:14:290)
    at async Promise.all (index 3)
    at async getConfig (./node_modules/storybook/builder-manager/dist/index.js:1:5670)
    at async Promise.all (index 1)
    at async getData (./node_modules/storybook/builder-manager/dist/index.js:1:4512)
    at async starter (./node_modules/storybook/builder-manager/dist/index.js:1:7579)

WARN Broken build, fix the error above.
WARN You may need to refresh the browser.

I followed the FAQ item in Chromatic's docs and by taking it's debugging steps I learnt that I needed at least one commit for Chromatic to work.

weeksling · 2023-10-03T23:43:53Z

@MichaelArestad I added the notification here with the same title and subheader but added a final callout to restart the storybook server. Does that seem alright to you?

tmeasday

LGTM and works well in testing.

I wonder if the notification would be weird for first time SB users who haven't even opened the addon yet. Perhaps we should play it cool and wait for them to reach the addon before showing them the message?

src/index.ts

Co-authored-by: Tom Coleman <[email protected]>

…start storybook

weeksling · 2023-10-04T17:38:15Z

I wonder if the notification would be weird for first time SB users who haven't even opened the addon yet. Perhaps we should play it cool and wait for them to reach the addon before showing them the message?

I agree. Removing the notification. I also updated it to keep the loop going so they don't have to restart storybook-server. The issue I thought I had with unsetting an addon state variable was an issue in my code

MichaelArestad · 2023-10-04T18:13:44Z

@MichaelArestad I added the notification here with the same title and subheader but added a final callout to restart the storybook server. Does that seem alright to you?

@weeksling In what scenario would folks see this? Is it necessary?

weeksling · 2023-10-04T19:05:50Z

@MichaelArestad I added the notification here with the same title and subheader but added a final callout to restart the storybook server. Does that seem alright to you?

@weeksling In what scenario would folks see this? Is it necessary?

No I don't think this is necessary. I removed the notification entirely. Users will see the error when they try to check the addon panel. I thought this was necessary to draw attention to, but it will be happening most often for new storybooks so it's noisy.

MichaelArestad · 2023-10-04T19:06:48Z

Perfect. Thank you!

weeksling added 2 commits October 3, 2023 18:46

Add GitNotFound error screen, try-catch, and state

3179a71

Add notification when git info error occurs (only happens once for now)

436a3c8

weeksling changed the title ~~Matt/ap 3618 git is required for visual test addon via chromatic cli~~ git is required for visual test addon via chromatic cli Oct 3, 2023

weeksling requested review from tmeasday and ghengeveld October 3, 2023 23:30

Add prompt to restart storybook in notification and fix lint errors

4816e15

weeksling marked this pull request as draft October 4, 2023 00:01

weeksling marked this pull request as ready for review October 4, 2023 00:01

tmeasday approved these changes Oct 4, 2023

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

src/index.ts Outdated Show resolved Hide resolved

Matthew Weeks and others added 3 commits October 4, 2023 13:01

Improve error logging on getGitInfo errors

57a113c

Co-authored-by: Tom Coleman <[email protected]>

remove notification on git errors to reduce noise

54c71eb

Continue looping and detect if git is fixed so users don't have to re…

d65e128

…start storybook

fix autofixable lint error

8f69032

Base automatically changed from tom/ap-3621-improve-first-build-error-handling to main October 4, 2023 19:20

weeksling merged commit 7385106 into main Oct 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

git is required for visual test addon via chromatic cli #126

git is required for visual test addon via chromatic cli #126

weeksling commented Oct 3, 2023 •

edited by tmeasday

Loading

linear bot commented Oct 3, 2023

Reproduction

weeksling commented Oct 3, 2023

tmeasday left a comment

weeksling commented Oct 4, 2023

MichaelArestad commented Oct 4, 2023

weeksling commented Oct 4, 2023

MichaelArestad commented Oct 4, 2023

git is required for visual test addon via chromatic cli #126

git is required for visual test addon via chromatic cli #126

Conversation

weeksling commented Oct 3, 2023 • edited by tmeasday Loading

linear bot commented Oct 3, 2023

Reproduction

weeksling commented Oct 3, 2023

tmeasday left a comment

Choose a reason for hiding this comment

weeksling commented Oct 4, 2023

MichaelArestad commented Oct 4, 2023

weeksling commented Oct 4, 2023

MichaelArestad commented Oct 4, 2023

weeksling commented Oct 3, 2023 •

edited by tmeasday

Loading