Design doc for #2158: Flag-Gated Code Coverage for Bosatsu Test Execution#2159
Design doc for #2158: Flag-Gated Code Coverage for Bosatsu Test Execution#2159johnynek wants to merge 3 commits into
Conversation
Summary of ChangesHello, 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 introduces a comprehensive design document outlining the implementation of optional, flag-gated code coverage for Bosatsu's test execution. The proposal details how to instrument code during lowering, collect execution hits at runtime, and generate CI-friendly reports in LCOV and Cobertura formats, all while minimizing performance impact and ensuring compatibility with existing workflows. Highlights
Changelog
Activity
Using Gemini Code AssistThe 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
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 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a design document for adding flag-gated code coverage to Bosatsu tests. The design is comprehensive, covering the architecture, implementation plan, testing strategy, and risks. It proposes instrumenting code during the Matchless lowering phase, collecting hit counts in the interpreter, and generating reports in LCOV and Cobertura formats. My main feedback concerns the interaction between probe registration and Matchless.Expr optimizations, which could affect the stability of probe identifiers.
| ### 2) Static Probe Registration During Matchless Lowering | ||
| Instrument at lowering time, not by changing Bosatsu language semantics: | ||
| 1. Extend `Matchless.fromLet` with optional coverage recorder callbacks. | ||
| 2. For each compiled typed-expression root that has usable source region metadata, register a probe id against the produced Matchless expression object identity. | ||
| 3. For each typed `Match`, additionally register a branch decision group with one alternative per branch body. | ||
|
|
||
| This avoids invasive AST-wide payload changes while still mapping execution to source regions. |
There was a problem hiding this comment.
The proposal to register probes against Matchless.Expr object identity is a key part of the design. However, there's a potential issue with Matchless.Expr optimizations.
The Matchless.fromLet function, where instrumentation is proposed, applies several optimizations at the end of its execution (e.g., reuseConstructors, hoistInvariantLoopLets). These optimizations rewrite the Matchless.Expr tree, which will invalidate any object identities registered before they run. This could lead to probes being associated with stale objects and result in incorrect coverage data.
To address this, the design should clarify the ordering. Either:
- Probe registration should occur after all
Matchless.Exproptimizations have been applied. - These
Matchless.Expr-level optimizations should be disabled when coverage collection is enabled, similar to howNoOptimizeis used forTypedExproptimizations.
There was a problem hiding this comment.
Please address the comment.
One idea could be to aggregrate a RegionSet for Expr values, so when we optimize two different source positions into a single bit of code, we can take that single bit with counting for executing two regions. Although, that's a hand waving argument.
There was a problem hiding this comment.
Addressed. I adopted the RegionSet direction in Static Probe Registration During Matchless Lowering: coverage metadata is now (sourceHash, RegionSet), and rewrites/merges explicitly union RegionSets so one optimized expression can count for multiple source regions.
Codecov Report✅ All modified and coverable lines are covered by tests. Please upload reports for the commit 6be5139 to get more accurate results. Additional details and impacted files@@ Coverage Diff @@
## main #2159 +/- ##
==========================================
+ Coverage 85.03% 85.39% +0.35%
==========================================
Files 190 184 -6
Lines 46891 38710 -8181
Branches 11805 9731 -2074
==========================================
- Hits 39876 33057 -6819
+ Misses 7015 5653 -1362 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| For each executed probe: | ||
| 1. Source package name. | ||
| 2. Source region (`start`, `end` offsets). | ||
| 3. Probe kind (`line` probe, `branch alternative` probe). |
There was a problem hiding this comment.
why isn't this implied by keeping track of the source regions executed? For instance if you execute the branch expr for a match then you have taken that branch. How is it different from the source region?
There was a problem hiding this comment.
Good point. I updated Coverage Data We Must Retain to make this explicit: executed regions do imply a branch was taken, and the extra decision-group metadata is only to preserve the denominator/untaken alternatives required by branch-report formats.
| ### 2) Static Probe Registration During Matchless Lowering | ||
| Instrument at lowering time, not by changing Bosatsu language semantics: | ||
| 1. Extend `Matchless.fromLet` with optional coverage recorder callbacks. | ||
| 2. For each compiled typed-expression root that has usable source region metadata, register a probe id against the produced Matchless expression object identity. | ||
| 3. For each typed `Match`, additionally register a branch decision group with one alternative per branch body. | ||
|
|
||
| This avoids invasive AST-wide payload changes while still mapping execution to source regions. |
There was a problem hiding this comment.
Please address the comment.
One idea could be to aggregrate a RegionSet for Expr values, so when we optimize two different source positions into a single bit of code, we can take that single bit with counting for executing two regions. Although, that's a hand waving argument.
|
|
||
| This avoids invasive AST-wide payload changes while still mapping execution to source regions. | ||
|
|
||
| ### 3) Runtime Hit Collection In Evaluator |
There was a problem hiding this comment.
we should not slow down the normal MatchlessToValue to do this. It should be purely opt-in. So, when we aren't running coverage, there is no hit on performance.
There was a problem hiding this comment.
Updated. The Runtime Hit Collection In Evaluator section now states a strict zero-cost normal path: non-coverage runs keep the existing MatchlessToValue path unchanged, and coverage uses a separate evaluator path only when --coverage is enabled.
|
|
||
| ### 4) Source Mapping And Filtering | ||
| Coverage should only emit files we can map back to source paths: | ||
| 1. Use `(Path, PackageName)` mapping already returned by compilation (`RuntimeCommandSupport.packMap`) to map package to source file path. |
There was a problem hiding this comment.
I think hashing the source with blake3 and keeping that as the key in Matchless source information could be a good idea. So HashValue[Algo.Blake3] and the RegionSet should be enough.
There was a problem hiding this comment.
Integrated this suggestion. The doc now uses HashValue[Algo.Blake3] as the source key and describes hash -> (path, LocationMap) resolution for reporting.
| 2. Update `RuntimeCommandSupport.packMap` to accept compile options from caller. | ||
| 3. Add coverage model classes (`CoverageModel`, `CoverageAggregator`) for static probes and runtime counters. | ||
| 4. Extend `Matchless.fromLet` and `MatchlessFromTypedExpr.compile` to register probes and branch groups during lowering. | ||
| 5. Extend `MatchlessToValue` with optional expression-entry callback. |
There was a problem hiding this comment.
again, I think we must be clear that we should have a zero cost impact on the normal path.
There was a problem hiding this comment.
Clarified again in both architecture and plan/acceptance sections: normal runs must not pay coverage overhead, and coverage logic is isolated to the explicit opt-in path.
|
Updated |
…ecoverage-for-bosatsu
|
Merged |
|
MergeXO feedback automation is blocked because the agent returned Action: request concrete file edits (or explicit |
1 participant
Design doc.
Refs #2158