Add context around file adds/moves in BaselineCheckoutFailedError bails

[codykaup]

This PR adds additional tags to our Sentry exceptions to add more context to our BaselineCheckoutFailedError error in TurboSnap. To do that, we try to determine if a field was added or moved since the baseline because checking out either will fail earlier in the TurboSnap diff process. Ideally, we would use this information to follow the file around instead of bailing but we'll start with data collection first.

I was using my sample monorepo project to test out each scenario. I'll attempt to describe how I QAed each below. It uses Nx with Yarn.

Note

This addition does NOT get sent to the backend. It's merely adding Sentry details.

To force these exceptions to go to Sentry, I patched the CLI to enable it:

diff --git a/node-src/errorMonitoring.ts b/node-src/errorMonitoring.ts
index f673ddac..987b1b4f 100644
--- a/node-src/errorMonitoring.ts
+++ b/node-src/errorMonitoring.ts
@@ -51,10 +51,8 @@ Sentry.init({
   release: process.env.SENTRY_RELEASE || process.env.npm_package_version,
   dist: process.env.SENTRY_DIST,
   sampleRate: 1,
-  environment: process.env.SENTRY_ENVIRONMENT,
-  enabled:
-    process.env.DISABLE_ERROR_MONITORING !== 'true' &&
-    process.env.SENTRY_ENVIRONMENT !== 'development',
+  environment: 'development',
+  enabled: true,
   enableTracing: false,
   integrations: [],
   initialScope: {

baselineManifestAdded

I added a new package to my monorepo and commited it. Then ran a build with TurboSnap enabled (--only-changed). Doing this, yielded the expected bail:

execGitCommand: git cat-file -e "ac63e5eb474358e6a3b1e9ee9a2a5ea5b436448a^{commit}"
execGitCommand result: ''
execGitCommand: git diff --name-status --find-renames=20% ac63e5eb474358e6a3b1e9ee9a2a5ea5b436448a HEAD -- ":(glob,top)**/package.json"
execGitCommand result: 'A  packages/lambda/package.json'
⚠ TurboSnap disabled due to file change
Found a package file change in packages/lambda/package.json or its module sibling yarn.lock
A full build is required because this file cannot be linked to any specific stories.
ℹ Read more at https://www.chromatic.com/docs/turbosnap#how-it-works

Then in Sentry:

baselineManifestMoved

Following baselineManifestAdded, I moved the lambda package to lambda2 then ran another build. Doing this, yielded the expected bail:

execGitCommand: git cat-file -e "8619400ab8ee0aaac20a6b1c39c5edd2b971cc50^{commit}"
execGitCommand result: ''
execGitCommand: git diff --name-status --find-renames=20% 8619400ab8ee0aaac20a6b1c39c5edd2b971cc50 HEAD -- ":(glob,top)**/package.json"
execGitCommand result: 'R100       packages/lambda/package.json    packages/lambda2/package.json'
⚠ TurboSnap disabled due to file change
Found a package file change in packages/lambda2/package.json or its module sibling yarn.lock
A full build is required because this file cannot be linked to any specific stories.
ℹ Read more at https://www.chromatic.com/docs/turbosnap#how-it-works

Then in Sentry:

unknownBaselineCheckoutFailure

For this one, I forced the error by patching the CLI:

diff --git a/node-src/lib/turbosnap/classifyBailRootCause.ts b/node-src/lib/turbosnap/classifyBailRootCause.ts
index ab345544..2d9a4fc8 100644
--- a/node-src/lib/turbosnap/classifyBailRootCause.ts
+++ b/node-src/lib/turbosnap/classifyBailRootCause.ts
@@ -51,6 +51,7 @@ async function classifyBaselineCheckoutFailureTags(
   if (!(error instanceof BaselineCheckoutFailedError)) {
     return undefined;
   }
+  return { baseline_failure_kind: 'unknownBaselineCheckoutFailure' };
 
   const { reference, fileName } = parsePathspec(error.pathspec);
 

Then I built the CLI, added a new package in my monorepo, then ran another TurboSnap build. Then I got the following bail reason:

⚠ TurboSnap disabled due to file change
Found a package file change in packages/lambda/package.json or its module sibling yarn.lock
A full build is required because this file cannot be linked to any specific stories.
ℹ Read more at https://www.chromatic.com/docs/turbosnap#how-it-works

Then in Sentry:

📦 Published PR as canary version: 17.5.0--canary.1391.27305254386.0

✨ Test out this PR locally via:

npm install chromatic@17.5.0--canary.1391.27305254386.0
# or 
yarn add chromatic@17.5.0--canary.1391.27305254386.0

[github-actions]

📦 Package Size: 7124 KB

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.00%. Comparing base (32938f1) to head (a43751b).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1391      +/-   ##
==========================================
+ Coverage   81.86%   82.00%   +0.14%     
==========================================
  Files         230      231       +1     
  Lines        4357     4391      +34     
  Branches     1251     1261      +10     
==========================================
+ Hits         3567     3601      +34     
  Misses        678      678              
  Partials      112      112              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[justin-thurman]

I haven't reviewed this in exhaustive detail yet, but I wanted to surface a few thoughts. I think the --no-relative one is blocking though. I'll circle back and review more thoroughly after you respond and/or make changes.

[justin-thurman]

getChangedFiles uses --no-relative in its git diff command. I think we need that here or running from a subdirectory would have bizarre behavior. Probably use --no-pager too, for consistency.

[justin-thurman]

Any concern about serialization of the error here? Would String(err) or message + stack be better?

[justin-thurman]

I think this will fail if we have a shallow clone because the git diff call inside getChangedFilesWithStatus will fail. We already have a commitExists function. Should we use that at the start of classifyBaselineCheckoutFailureTags to see if the commit doesn't exist, and if it doesn't, throw a new bail reason?

I know there's been talk around shallow clones lately, so they might be handled elsewhere, making this a moot point. But I can't recall for sure, so figured I'd raise it.