Skip to content

test(cl): run ';' expect cases and skip output diff only#1743

Open
luoliwoshang wants to merge 1 commit intogoplus:mainfrom
luoliwoshang:fix/cltest-semicolon-run-only
Open

test(cl): run ';' expect cases and skip output diff only#1743
luoliwoshang wants to merge 1 commit intogoplus:mainfrom
luoliwoshang:fix/cltest-semicolon-run-only

Conversation

@luoliwoshang
Copy link
Copy Markdown
Member

Fixes #1742

Summary

  • change testRunFrom so expect.txt == ';' means run-only / no-diff instead of early return
  • keep run/build failure reporting unchanged
  • add cl/cltest/cltest_test.go regression test to ensure semicolon-marked cases still execute

Behavior Change

Before:

  • expect.txt == ';' skipped execution entirely

After:

  • test still executes RunAndCapture*
  • output diff is skipped for this marker

Validation

  • go test ./cl/cltest -run TestTestRunFromSemicolonStillRuns -count=1 -v
  • go test ./cl -run TestFilterEmulatorOutput -count=1

@gemini-code-assist
Copy link
Copy Markdown

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refactors the testRunFrom function within the cltest package to alter the interpretation of an expect.txt file containing only a semicolon. Previously, this marker would cause the test to be entirely skipped. Now, the test will execute as normal, but the output comparison step will be bypassed. This change addresses an issue where tests intended to run without output validation were being skipped, improving the robustness of the testing framework by ensuring all relevant tests are executed.

Highlights

  • Test Execution Logic: Updated the testRunFrom function to change the behavior of expect.txt containing a semicolon. Previously, this would skip the entire test execution; now, it only skips the output diff, allowing the test to run.
  • Testability Improvements: Introduced package-level variables for RunAndCapture and RunAndCaptureWithConf to allow these functions to be mocked, improving the testability of cltest.
  • Regression Test Added: Added a new regression test, TestTestRunFromSemicolonStillRuns, to ensure that tests marked with a semicolon in expect.txt are indeed executed.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request correctly changes the behavior for test cases where expect.txt contains a semicolon, so that they are executed but their output is not compared. The change is well-implemented by moving the check after the test execution. The addition of a regression test is excellent for ensuring this new logic is preserved.

I have one suggestion regarding the test implementation strategy. The use of package-level variables for mocking, while effective, can limit the ability to run tests in parallel. I've added a comment with more details on this point for future consideration.

@xgopilot
Copy link
Copy Markdown
Contributor

xgopilot bot commented Mar 24, 2026

The behavioral change is sound — semicolon-marked tests now actually execute and catch runtime failures instead of being silently skipped. The main concern is the package-level mutable function variables introduced for test mocking: they create a data race risk under parallel test execution and add indirection that's only half-used. Consider dependency injection via runOptions instead. A couple of minor test coverage gaps noted inline.

@luoliwoshang
Copy link
Copy Markdown
Member Author

-- FAIL: TestRunFromTestlibgo (145.71s)
2026-03-24T02:58:21.4033610Z --- FAIL: TestRunFromTestlibgo/errors (10.71s)
2026-03-24T02:58:21.4034530Z cltest.go:235: raw output:
2026-03-24T02:58:21.4034920Z panic: error
2026-03-24T02:58:21.4035520Z
2026-03-24T02:58:21.4035850Z [0x044A47F0 command-line-arguments.main+0x1b, SP = 0x2c]
2026-03-24T02:58:21.4036340Z [0x044A4780 main+0x4, SP = 0x24]
2026-03-24T02:58:21.4036800Z cltest.go:236: run failed: run failed: exit status 2
2026-03-24T02:58:21.4037110Z output: panic: error
2026-03-24T02:58:21.4037550Z
2026-03-24T02:58:21.4038070Z [0x044A47F0 command-line-arguments.main+0x1b, SP = 0x2c]
2026-03-24T02:58:21.4038490Z [0x044A4780 main+0x4, SP = 0x24]
2026-03-24T02:58:21.4038850Z --- FAIL: TestRunFromTestlibgo/sync (13.09s)
2026-03-24T02:58:21.4039750Z cltest.go:235: raw output:

@luoliwoshang
Copy link
Copy Markdown
Member Author

2026-03-24T02:58:21.4140780Z cltest.go:236: run failed: run failed: exit status 1
2026-03-24T02:58:21.4141270Z output: ld64.lld: error: undefined symbol: internal/reflectlite.TypeOf
2026-03-24T02:58:21.4142730Z >>> referenced by /var/folders/t5/f77_gwnj6p95qxy9py3fckx00000gn/T/pkg-486331513.a(363d862c1f1a667a6ea53824c61098f62e67898bdb1aa7825b530ca365477496-d-2299899411.o):(symbol errors.init+0x64)
2026-03-24T02:58:21.4143340Z
2026-03-24T02:58:21.4143550Z ld64.lld: error: undefined symbol: internal/reflectlite.init
2026-03-24T02:58:21.4144230Z >>> referenced by /var/folders/t5/f77_gwnj6p95qxy9py3fckx00000gn/T/pkg-486331513.a(363d862c1f1a667a6ea53824c61098f62e67898bdb1aa7825b530ca365477496-d-2299899411.o):(symbol errors.init+0x2c)
2026-03-24T02:58:21.4144960Z clang++: error: linker command failed with exit code 1 (use -v to see invocation)

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 25, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.16%. Comparing base (aae5e97) to head (9dc6bd9).
⚠️ Report is 19 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1743      +/-   ##
==========================================
+ Coverage   93.15%   93.16%   +0.01%     
==========================================
  Files          48       48              
  Lines       13352    13349       -3     
==========================================
- Hits        12438    12437       -1     
+ Misses        727      726       -1     
+ Partials      187      186       -1     

☔ View full report in Codecov by Sentry.
📢 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.

@luoliwoshang luoliwoshang force-pushed the fix/cltest-semicolon-run-only branch from 6382160 to 9dc6bd9 Compare March 25, 2026 15:44
@luoliwoshang luoliwoshang requested a review from visualfc March 26, 2026 03:36
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.

test(cl): when expect.txt is ';', skip diff only, not execution

2 participants