Skip to content

Conversation

@jakebailey
Copy link
Member

This does two things:

  • Uses go test -json to stream the output in a parseable format.
  • With the parsed output, read each failing test's output to see if all of its failures are due to missing baselines.

We have no tests where this matters (I or others had fixed them previously), but I have a change where it does matter in the future.

Copilot AI review requested due to automatic review settings December 11, 2025 22:26
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR modernizes the updateFailing script by switching from synchronous test execution with regex parsing to streaming JSON output from go test -json. The key improvement is the ability to distinguish between baseline-only failures (which shouldn't mark tests as failing) and genuine test failures, enabling more accurate tracking of truly broken tests.

Key changes:

  • Migrated from execFileSync to spawn with streaming JSON output parsing
  • Added logic to differentiate baseline failures from real test errors by analyzing test output
  • Simplified the CI workflow by removing redundant test and baseline-accept steps

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
internal/fourslash/_scripts/updateFailing.mts Completely refactored from synchronous regex-based parsing to async streaming with JSON parsing, added baseline-only failure detection logic
.github/workflows/ci.yml Removed redundant baseline-accept and test steps that are now handled more efficiently by the updated script

Comment on lines 89 to 91
/Error|error/i.test(outputWithoutBaseline) ||
/fatal|Fatal/.test(outputWithoutBaseline) ||
/Unexpected/.test(outputWithoutBaseline);
Copy link

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error detection regex patterns may produce false positives. For example, /Error|error/i will match any occurrence of these words in test output, including legitimate uses in variable names, comments, or expected error messages being tested. Consider using more specific patterns that match actual test failure indicators, or anchor the patterns more carefully to avoid matching these common words in benign contexts.

Suggested change
/Error|error/i.test(outputWithoutBaseline) ||
/fatal|Fatal/.test(outputWithoutBaseline) ||
/Unexpected/.test(outputWithoutBaseline);
/^\s*(Error:|ERROR:|error:)\b/m.test(outputWithoutBaseline) ||
/^\s*(FATAL:|Fatal:|fatal:)\b/m.test(outputWithoutBaseline) ||
/^\s*Unexpected\b/m.test(outputWithoutBaseline);

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be good to fix, yeah. There's not really a clear format

@jakebailey jakebailey marked this pull request as draft December 12, 2025 00:28
@jakebailey
Copy link
Member Author

I need to think about this harder; I have to go manually merge this into one of my branches to get it to test so it's annoying to figure out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants