[WIP] Feature/more detailed lcov#275
Draft
chrisdp wants to merge 34 commits into
Draft
Conversation
chrisdp
commented
Feb 12, 2024
Open
chrisdp
commented
Sep 4, 2024
chrisdp
commented
Sep 4, 2024
Member
|
@chrisdp what needs done yet before this or can merge? I'm sure many people would appreciate having this current functionality soon. Is there anything that could be landed now and then other things implemented in future PRs? |
Collaborator
Author
|
@TwitchBronBron i think it might be close enough to go after we merge the file structure update, mocha, @only bug fixes, and promises support. Mostly because the code cov logic will also likely need fixes due to changes in brighterscript 0.68+. Most of the ast issues we had earlier were due to code cov. So we will want to at least fix that much before we merge. |
…ed-lcov # Conflicts: # bsc-plugin/package-lock.json # bsc-plugin/package.json # framework/src/components/rooibos/CodeCoverage.xml # framework/src/source/rooibos/CodeCoverage.brs # framework/src/source/rooibos/Coverage.bs # framework/src/source/rooibos/TestRunner.bs # package-lock.json # package.json # src/lib/rooibos/CodeCoverageProcessor.spec.ts # src/lib/rooibos/CodeCoverageProcessor.ts # src/plugin.spec.ts # tests/bsconfig.json # tests/package-lock.json # tests/package.json
…I capture, HTML viewer) Framework instrumentation rewrite for proper LCOV output and an end-to-end report pipeline that produces an nyc-style HTML viewer. CodeCoverageProcessor: - Emit 1-indexed line numbers (LCOV/Istanbul convention) instead of 0-indexed. - Defer reportFunction insertion until after the AST walk completes; inserting during the walk shifted owner[0] mid-visit and caused the visitor to skip the first statement's children (returns inside if/else were silently lost). - Pair an IfStatement's then/else blocks under one block.id with the if-statement's anchor line, so consumers can render them as a single decision with multiple outcomes. - Anchor for/while/for-each body branches to the loop statement line, not the body's first line. - coverageExcludedFiles matcher is now case-insensitive (`Main.bs` matches `**/source/main.bs`), and `**/components/rooibos/**/*` is excluded by default (the framework's own coverage Task moved to that path). Coverage.bs runtime: - Always emit FNDA/BRDA rows including unhit ones (LCOV spec) so missed functions and branches render in HTML. - Synthesize an implicit-else BRDA for single-arm ifs from condition_evaluations - then_hits, computed at lcov-write time. Lets the HTML viewer flag never-taken falsy paths without instrumenting both arms. CLI: - New --coverage-output flag; the console-output listener captures everything between `+-=-coverage:start`/`+-=-coverage:end` and writes lcov.info with SF: paths rewritten to absolute against bsConfig.rootDir. scripts/lcov-to-html.js: - Pure-JS lcov -> Istanbul HTML renderer using lcov-parse + istanbul-lib-coverage + istanbul-lib-report + istanbul-reports (all already pulled in transitively by nyc, no new deps). - Groups BRDA rows by block to produce multi-location branch entries with type:'if', so the annotator emits inline I/E badges. - Reads source files to compute the indent column for each branch line so badges anchor next to the statement (matching nyc's TS reports). - Strips the hardcoded `prettyprint lang-js` class from generated pages (BS isn't JS; prettify mis-tokenized the source and overrode the badge color). coverage-sample/: - New benchmark project separate from test-project. Calculator.bs is designed to be 100%-coverable; PartialCoverage.bs has deliberate hits and misses across paired if/else, single-arm if, loop bodies, and a never-called function. package.json: - npm scripts: build-coverage-sample, coverage:run, coverage:html, coverage:report. Tests: 120 -> 125 passing, 1 failing -> 0 failing. Three new regression tests cover return instrumentation in top-level/namespaced/class-method contexts. Existing snapshot tests updated for the new line numbering and paired-branch IDs.
Pair try and catch blocks under a single block.id (same model used for if/else), anchor both arms to the `try` keyword line, and instrument the try line itself by prefixing the `try` token with a reportLine call - matching the token-text-mutation pattern already used for for/while/ forEach (avoids the walker re-read bug that breaks arraySplice insertions mid-visit). Sample updates: - Calculator.safeDivide: both try and catch arms exercised by spec - PartialCoverage.parseSafe: only try succeeds, catch arm missed (E badge)
Adds expression-level branch coverage for ternary expressions, with the missed arm rendered exactly like nyc/Istanbul TS reports - yellow cbranch-no wrap on the unhit arm. Framework: - Defer convertStatementToCoverageStatement (the reportLine inserts) until after the walk completes. The pre-existing inline arraySplice was breaking the walker's descent into statement children, so a ternary inside a return statement was never visited. Same kind of issue we hit earlier with reportFunction inserts. - New TernaryExpression visitor that wraps each arm in a RBS_CC_<id>_branchValue(blockId, branchId, value) helper call, parsed from a placeholder template and grafted with the original arm as a sub-expression so the walker can still descend into nested ternaries. - Added matching branchValue helper to coverageBrsTemplate (pass-through that records the hit and returns the value unchanged). - BranchCoverage.branches now has optional column / endColumn for expression-level branches; ternary visitor records both arms' ranges. Coverage.bs runtime emits a custom RBSCOL:<block>,<branch>,<startCol>,<endCol> line per branch when columns are present. Non-LCOV but lcov-parse ignores unknown lines. Renderer: - Pre-parses RBSCOL lines (handles CRLF from device console capture) into a sidecar map; strips them before handing to lcov-parse. - When a branch has column data, builds the branchMap entry with type:'cond-expr' and exact start/end columns so Istanbul's annotator fires the wrap path with cbranch-no (yellow !important) - exactly how TS reports render missed ternary arms. Block-level branches without column data continue to use type:'if' for the badge approach (the wrap path's snap-to-whitespace logic mis-handles full-line wraps). Sample updates: - Calculator.abs: ternary with both arms exercised - no badge - PartialCoverage.asLabel: ternary truthy arm only - "off" wrapped yellow Tests: 5 snapshot expectations updated to include the new branchValue helper definition. 125 passing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Mirrors the ternary visitor: NullCoalescingExpression's consequent and alternate are each wrapped with RBS_CC_<id>_branchValue(...) so the renderer can highlight a missed alternate in yellow exactly the way it does for ternary arms. Note on semantics: BrightScript always evaluates the consequent of `??` to null-check it, so branch 0 (consequent) fires on every reach rather than only when the consequent is "kept". The visually meaningful case - "alternate never tested" - is still captured directly via branch 1. Sample updates: - Calculator.withDefault: both arms exercised - PartialCoverage.nameOrDefault: only consequent exercised, alternate rendered in yellow
Adds a BinaryExpression visitor that wraps each operand of a logical `and`/`or` with RBS_CC_<id>_branchValue. Short-circuit semantics are preserved naturally: when BS short-circuits the right operand, BS never evaluates the right wrap, so branch 1 stays unhit and the renderer flags it in yellow. Bitwise integer use (e.g. `5 and 3`) still works - both wraps fire because BS evaluates both sides for integers, so both arms show as hit and there are no false-positive missed-branch reports. The synthetic AND that the IfStatement handler injects to wrap an if's condition with a reportLine call is added to processedExpressions so the new BinaryExpression visitor doesn't try to instrument it again. Also unskips `correctly transpiles some statements` (anonymous function expressions) - that test now passes incidentally because the same fixes that made expression-level branches reachable (deferring convertStatementToCoverageStatement and reportFunction inserts so the walker can descend into statement children) also let the walker reach nested function expressions and instrument their bodies. Sample updates: - Calculator.isInRange: logical AND, both short-circuit and full-eval paths exercised - PartialCoverage.isTrue: logical OR right side never evaluated, renders yellow on `fallback` Tests: 126 passing, 1 pending (unrelated pre-existing skip). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Anon functions already work (the deferred-inserts fix in the earlier commit unblocked walking into FunctionExpression bodies); this just adds samples that exercise the rendering paths and tightens the fstat-no wrap to match TS reports. Sample updates: - Calculator.classifySign: assigned anon function with both arms hit - PartialCoverage.describeFlag: anon function called once with the else arm missed - inner `if f then` shows the E badge - PartialCoverage.unusedHandler: anon returned but never invoked - FNDA:0,bench.unusedHandler$anon0 with body lines as cstat-no Renderer: - New getKeywordColumn helper finds the `function`/`sub` keyword on the declaration line; fnMap.decl now starts at that column instead of col 0. The fstat-no highlight wraps just `function(args) as ret` rather than the whole assignment line `handler = function(...)`, matching nyc/Istanbul TS reports. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
CI cleanup in CodeCoverageProcessor.ts: - Remove the dead transpileState field (and BrsTranspileState import) - it was leftover from an earlier wrap-via-transpile approach; the current template-parsing path doesn't need it. - Drop unused funcId locals in the report-text helpers; the call to ensureFunctionTracked is kept for the side effect of registering the function. - Strip ~240 lines of dead code at the file tail (unused createCovMap function and a Perl reference dump that was commented out). Naming cleanup: - Rename getFunctionIdInFile -> ensureFunctionTracked since callers consume the side effect (registration + queued reportFunction insert), not the return value. Drop the now-dead return value and the unused owner/key parameters that were forwarded but never consumed. New tests for coverage + global mocking interaction: - Both helper sets and the mock prologue end up in the output. - Returns and ternary arms inside a mocked function still get instrumented end-to-end. - The mock prologue's anonymous lookup function isn't accidentally registered as a user-defined function for tracking. Tests: 126 -> 129 passing, 1 pending. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Merge the two `from 'brighterscript'` imports into one type-import +
one runtime-import (eslint flagged the same module being imported on
multiple lines).
- Remove the vestigial `try {} catch (e) { console.log('Error:', e.stack) }`
in the constructor - empty try body, so the catch could never fire,
and `e.stack` ran into noImplicitAny on `unknown` errors.
- Spell out the longform property syntax in the pendingLineReports.push
call to satisfy `object-shorthand: never`.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Open
Apps with their own build pipeline can run rooibos coverage without asking the CLI to also build. Skips the builder.run() step when --package is set; deploys the supplied artifact and watches the device console. Accepts either: - A .zip - deployed as-is via rokuDeploy.publish - A directory - zipped to out/rooibos-prebuilt.zip via rokuDeploy.zipFolder first (most build systems write a folder of files, not a zip) The user is responsible for having built the package with the rooibos plugin enabled - otherwise the device runs unmodified code and the coverage capture flow has nothing to scrape. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Context for picking up the coverage work in a new session: where the feature branch is (TS-Istanbul parity for line/function/branch coverage including ternary, ??, and logical short-circuit), the unblocked production-app integration blocker (Roku's per-function bytecode cap trips on big functions because the per-line/per-branch report calls double or triple function size), and a proposed direction (auto-split function bodies into helper chunks at instrumentation time, with a skip-large-functions escape hatch as a fallback).
rokuDeploy.zipFolder defaults to including every file in the source dir, which dragged .brs.map and .xml.map files into the deployed channel. The maps are harmless functionally but bloat channel size (saw real apps trip Roku's package-size warnings just from this), and Roku has no use for them anyway. Pass an explicit filter that keeps everything except *.map files.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adding support for more detailed lcov file reporting to include things like function coverage.
Example coverage from test app in repo:
