Conversation
This commit introduces code coverage tracking to the testing framework. It includes new Makefile targets, updates to the README, and the implementation of core coverage classes for instrumentation, tracking, and reporting. Co-authored-by: kyle_kincer <kyle_kincer@sweetwater.com>
WalkthroughAdds a line-level code coverage subsystem to the 4D Unit Testing Framework: tracker, instrumenter, reporter, runtime record routine, TestRunner integration, Makefile coverage targets, documentation, and unit tests for instrumentation, collection, reporting, and restoration. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TestRunner
participant CodeInstrumenter
participant TestExecution
participant CoverageTracker
participant CoverageReporter
rect `#EFEFEF`
User->>TestRunner: run tests (--user-param coverage)
end
TestRunner->>CoverageTracker: initialize()/cleanup()
TestRunner->>CodeInstrumenter: instrumentMethods(targets)
CodeInstrumenter->>CodeInstrumenter: save original code + inject recordLine calls
Note over CodeInstrumenter: instrumented methods written back
TestRunner->>TestExecution: execute instrumented tests
loop per executed line
TestExecution->>CoverageTracker: recordLine(method, lineNum)
CoverageTracker->>CoverageTracker: increment counter in shared storage
end
TestExecution-->>TestRunner: tests complete
TestRunner->>CoverageTracker: collectData()
TestRunner->>CodeInstrumenter: restoreOriginalCode()
TestRunner->>CoverageReporter: generateReport(format)
CoverageReporter->>CoverageReporter: render -> text/json/html/lcov
CoverageReporter-->>User: write report(s)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
COVERAGE.md(1 hunks)COVERAGE_IMPLEMENTATION.md(1 hunks)Makefile(3 hunks)README.md(2 hunks)docs/coverage-guide.md(1 hunks)testing/Project/Sources/Classes/CodeInstrumenter.4dm(1 hunks)testing/Project/Sources/Classes/CoverageReporter.4dm(1 hunks)testing/Project/Sources/Classes/CoverageTest.4dm(1 hunks)testing/Project/Sources/Classes/CoverageTracker.4dm(1 hunks)testing/Project/Sources/Classes/TestRunner.4dm(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
testing/Project/Sources/Classes/**/*.4dm
📄 CodeRabbit inference engine (CLAUDE.md)
testing/Project/Sources/Classes/**/*.4dm: Name 4D test classes with the suffix "Test" for auto discovery (e.g., ExampleTest.4dm)
Name test methods with the prefix "test_" so the framework recognizes and runs them
Use comment-based tagging in tests with "// #tags: tag1, tag2" to enable filtering
Opt out of parallel execution for a suite by adding "// #parallel: false" at the test class level
Disable automatic transactions for a test by adding "// #transaction: false" and manage transactions manually if needed
When manual control is required, use Testing context helpers: startTransaction, inTransaction, validateTransaction, cancelTransaction, withTransaction, withTransactionValidate
Control trigger behavior per test with annotations: "// #triggers: enabled" or "// #triggers: disabled"
Place 4D test classes under testing/Project/Sources/Classes/ so the framework can discover and run them
Tag integration tests explicitly with "// #tags: integration" (and enable triggers when needed) to support filtering
Files:
testing/Project/Sources/Classes/CodeInstrumenter.4dmtesting/Project/Sources/Classes/CoverageTracker.4dmtesting/Project/Sources/Classes/CoverageTest.4dmtesting/Project/Sources/Classes/TestRunner.4dmtesting/Project/Sources/Classes/CoverageReporter.4dm
testing/Project/Sources/Classes/*Test.4dm
📄 CodeRabbit inference engine (AGENTS.md)
testing/Project/Sources/Classes/*Test.4dm: Name test classes so their file names end with "Test.4dm" to enable auto discovery
Within test classes, name test methods with the prefix "test_" so they are executed by the runner
Use comment-based tagging with lines like "// #tags: unit, integration, slow" to categorize tests
Annotate suites that must not run in parallel with "// #parallel: false"
Disable automatic transactions for specific tests by adding "// #transaction: false" and manage transactions manually if needed
Tag tests that should not run on Linux (e.g., "no-linux") so they can be excluded with "excludeTags=no-linux" on Linux runners
Files:
testing/Project/Sources/Classes/CoverageTest.4dm
Makefile
📄 CodeRabbit inference engine (CLAUDE.md)
Provide and use the documented Make targets (e.g., test, test-json, test-junit, test-ci, test-parallel, test-unit, test-integration) for consistent test execution
Files:
Makefile
🧠 Learnings (11)
📚 Learning: 2025-09-22T13:15:15.427Z
Learnt from: CR
Repo: KyleKincer/testing PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T13:15:15.427Z
Learning: Applies to testing/Project/Sources/Classes/*Test.4dm : Use comment-based tagging with lines like "// #tags: unit, integration, slow" to categorize tests
Applied to files:
README.mdtesting/Project/Sources/Classes/CoverageTest.4dmtesting/Project/Sources/Classes/TestRunner.4dm
📚 Learning: 2025-10-20T18:19:46.650Z
Learnt from: CR
Repo: KyleKincer/testing PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-20T18:19:46.650Z
Learning: Applies to testing/Project/Sources/Classes/**/*.4dm : Use comment-based tagging in tests with "// #tags: tag1, tag2" to enable filtering
Applied to files:
README.mdtesting/Project/Sources/Classes/TestRunner.4dm
📚 Learning: 2025-10-20T18:19:46.650Z
Learnt from: CR
Repo: KyleKincer/testing PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-20T18:19:46.650Z
Learning: Applies to testing/Project/Sources/Classes/**/*.4dm : When manual control is required, use Testing context helpers: startTransaction, inTransaction, validateTransaction, cancelTransaction, withTransaction, withTransactionValidate
Applied to files:
README.md
📚 Learning: 2025-10-20T18:19:46.650Z
Learnt from: CR
Repo: KyleKincer/testing PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-20T18:19:46.650Z
Learning: Applies to testing/Project/Sources/Classes/**/*.4dm : Tag integration tests explicitly with "// #tags: integration" (and enable triggers when needed) to support filtering
Applied to files:
README.mdtesting/Project/Sources/Classes/CoverageTest.4dmtesting/Project/Sources/Classes/TestRunner.4dm
📚 Learning: 2025-10-20T18:19:46.650Z
Learnt from: CR
Repo: KyleKincer/testing PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-20T18:19:46.650Z
Learning: Applies to testing/Project/Sources/Classes/**/*.4dm : Name test methods with the prefix "test_" so the framework recognizes and runs them
Applied to files:
README.mdtesting/Project/Sources/Classes/TestRunner.4dm
📚 Learning: 2025-10-20T18:19:46.650Z
Learnt from: CR
Repo: KyleKincer/testing PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-20T18:19:46.650Z
Learning: Applies to testing/Project/Sources/Classes/**/*.4dm : Control trigger behavior per test with annotations: "// #triggers: enabled" or "// #triggers: disabled"
Applied to files:
testing/Project/Sources/Classes/CoverageTest.4dmtesting/Project/Sources/Classes/TestRunner.4dm
📚 Learning: 2025-10-20T18:19:46.650Z
Learnt from: CR
Repo: KyleKincer/testing PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-20T18:19:46.650Z
Learning: Applies to Makefile : Provide and use the documented Make targets (e.g., test, test-json, test-junit, test-ci, test-parallel, test-unit, test-integration) for consistent test execution
Applied to files:
Makefile
📚 Learning: 2025-10-20T18:19:46.650Z
Learnt from: CR
Repo: KyleKincer/testing PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-20T18:19:46.650Z
Learning: Applies to testing/Project/Sources/Classes/**/*.4dm : Opt out of parallel execution for a suite by adding "// #parallel: false" at the test class level
Applied to files:
Makefile
📚 Learning: 2025-09-22T13:15:15.427Z
Learnt from: CR
Repo: KyleKincer/testing PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T13:15:15.427Z
Learning: Applies to testing/Project/Sources/Classes/*Test.4dm : Within test classes, name test methods with the prefix "test_" so they are executed by the runner
Applied to files:
testing/Project/Sources/Classes/TestRunner.4dm
📚 Learning: 2025-10-20T18:19:46.650Z
Learnt from: CR
Repo: KyleKincer/testing PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-20T18:19:46.650Z
Learning: Applies to testing/Project/Sources/Classes/**/*.4dm : Place 4D test classes under testing/Project/Sources/Classes/ so the framework can discover and run them
Applied to files:
testing/Project/Sources/Classes/TestRunner.4dm
📚 Learning: 2025-10-20T18:19:46.650Z
Learnt from: CR
Repo: KyleKincer/testing PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-20T18:19:46.650Z
Learning: When running tests from a host project, pass the host project's Storage to Testing_RunTestsWithCs to enable trigger control propagation
Applied to files:
testing/Project/Sources/Classes/TestRunner.4dm
🪛 checkmake (0.2.2)
Makefile
[warning] 216-216: Missing required phony target "all"
(minphony)
[warning] 216-216: Missing required phony target "clean"
(minphony)
🪛 markdownlint-cli2 (0.18.1)
README.md
111-111: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
COVERAGE.md
111-111: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
251-251: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
256-256: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
261-261: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
266-266: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
271-271: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
COVERAGE_IMPLEMENTATION.md
111-111: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
251-251: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
256-256: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
261-261: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
266-266: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
271-271: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/coverage-guide.md
60-60: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
79-79: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
89-89: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
116-116: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
126-126: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
137-137: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
193-193: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
282-282: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (22)
README.md (1)
74-194: Excellent documentation for the new coverage feature.The Code Coverage section is comprehensive, well-structured, and provides clear examples for all supported formats. The parameter table, usage examples, and CI/CD integration guidance will help users effectively leverage the coverage functionality.
COVERAGE_IMPLEMENTATION.md (1)
1-366: Well-documented implementation summary.This document provides an excellent technical overview of the coverage implementation, including deliverables, key features, usage examples, and design decisions. The statistics and references make it easy to understand the scope and integration points of the coverage system.
docs/coverage-guide.md (1)
1-691: Comprehensive coverage guide with excellent examples.This user guide is thorough and well-organized, covering everything from quick start to advanced usage, CI/CD integration, and troubleshooting. The examples for each report format, configuration options, and best practices will help users effectively adopt coverage tracking.
Makefile (2)
130-157: Well-structured coverage targets.The new Makefile targets for coverage follow the existing naming conventions and provide good coverage of different use cases (basic, HTML, LCOV, JSON, unit, integration). The targets correctly delegate to the base test command with appropriate coverage parameters.
179-211: Clear help documentation for coverage commands.The updated help output clearly documents all the new coverage targets with practical examples, making it easy for users to discover and use the coverage functionality.
testing/Project/Sources/Classes/TestRunner.4dm (8)
13-17: Clean property declarations for coverage support.The new coverage-related properties are well-typed and clearly named, following the existing property declaration pattern in the class.
29-29: Coverage initialization integrated into constructor.The call to
_initializeCoverage()is appropriately placed in the constructor, ensuring coverage configuration is parsed early in the test runner lifecycle.
36-48: Proper coverage lifecycle management in run().The coverage setup and teardown logic is correctly guarded by
coverageEnabledchecks and properly positioned around the main test execution. The setup occurs before_runInternal()and teardown occurs after, with proper cleanup even if tests fail.
1087-1104: Coverage initialization logic is sound.The
_initializeCoverage()method correctly:
- Initializes coverage to disabled by default
- Checks for coverage enablement via user parameters
- Initializes all required components (tracker, instrumenter)
- Parses coverage method patterns
1142-1160: Method filtering logic appropriately excludes test and framework methods.The
_shouldTrackMethodForCoverage()method correctly filters out:
- Test methods (patterns
@Test@andTest@)- Framework methods (
Testing_@,TestErrorHandler,TestGlobalErrorHandler)- Private methods (starting with
_)This ensures coverage focuses on the code under test rather than the test infrastructure.
1162-1182: Instrumentation setup with appropriate logging.The
_setupCoverage()method properly:
- Guards with
coverageEnabledcheck- Initializes the tracker
- Instruments discovered methods
- Logs instrumentation results in human format
- Reports both success and failure counts
1184-1200: Proper coverage teardown with cleanup.The
_teardownCoverage()method correctly:
- Collects coverage data
- Restores original code (critical for safety)
- Generates reports
- Cleans up storage
The restoration of original code is particularly important and is properly sequenced.
1202-1234: Flexible coverage report generation.The
_generateCoverageReport()method provides good flexibility:
- Supports multiple formats via user parameters
- Defaults to text format
- Supports both file output and console display
- Includes coverage stats in test results
- Logs output location for file-based reports
testing/Project/Sources/Classes/CoverageTest.4dm (1)
1-286: Comprehensive test suite for coverage functionality.The test suite thoroughly validates all major coverage components:
- CoverageTracker lifecycle, line recording, statistics, and data merging
- CodeInstrumenter line detection and indentation handling
- CoverageReporter format generation (text, JSON, HTML, LCOV)
All tests follow proper conventions:
- Named with
test_prefix- Tagged appropriately with
unit, coverage- Proper cleanup after each test
- Clear, focused assertions
The test coverage for the coverage system itself is excellent and will help ensure reliability of this critical functionality.
COVERAGE.md (1)
1-309: Excellent technical documentation for the coverage implementation.This document provides thorough technical documentation covering:
- Architecture and core components
- Instrumentation strategy with clear examples
- Data flow and lifecycle
- Well-justified design decisions
- Performance considerations and optimization strategies
- Current limitations and known issues
- Integration points with TestRunner and CI/CD
- Best practices for both users and developers
The documentation will be invaluable for both users trying to understand the system and developers who may need to maintain or extend it in the future.
testing/Project/Sources/Classes/CoverageTracker.4dm (7)
9-13: Clean constructor initialization.The constructor properly initializes all properties with appropriate default values.
15-25: Thread-safe shared storage initialization.The
initialize()method correctly usesUse (Storage)to ensure thread-safe initialization of the shared coverage data structure. The timestamp capture is appropriate for duration tracking.
26-44: Thread-safe line execution recording.The
recordLine()method properly:
- Validates storage exists before proceeding
- Uses nested
Useblocks for thread safety- Handles method initialization on first access
- Accumulates execution counts correctly
- Converts line numbers to strings for object keys
46-56: Safe data collection from shared storage.The
collectData()method:
- Captures end time for duration calculation
- Validates storage exists
- Uses
OB Copywithck sharedto safely copy from shared storage- Preserves existing data during copy
77-115: Coverage statistics calculation is correct.The
getCoverageStats()method properly:
- Iterates through all methods and their lines
- Counts total and covered lines
- Calculates coverage percentage with division-by-zero protection
- Returns comprehensive statistics including duration
The calculation logic is sound and handles edge cases appropriately.
157-176: Uncovered line identification with proper sorting.The
getUncoveredLines()method correctly:
- Returns empty collection for null coverage
- Identifies lines with zero execution count
- Converts string keys back to integers
- Sorts results for consistent output
178-195: Robust data merging for parallel execution.The
mergeData()method properly handles:
- New methods not yet tracked
- Existing methods requiring count accumulation
- Line-by-line count merging
This enables coverage data to be correctly combined from multiple test workers in parallel execution scenarios.
Co-authored-by: kyle_kincer <kyle_kincer@sweetwater.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
testing/Project/Sources/Classes/CodeInstrumenter.4dm (2)
87-95: CRITICAL: Restore methods even when original code is empty.The
restoreOriginalCodemethod skips methods whoseoriginalCodeis an empty string, leaving injected tracking code in place. This corrupts host methods that were intentionally blank (e.g., placeholders, empty interfaces) after a coverage run.Always restore the stored original code regardless of whether it's empty, and surface any METHOD SET CODE errors.
Apply this diff:
- If ($originalCode#"") - var $errorCode : Integer - METHOD SET CODE($methodPath; $originalCode; *; $errorCode) - - If ($errorCode#0) - $success:=False - End if - End if + var $errorCode : Integer + METHOD SET CODE($methodPath; $originalCode; *; $errorCode) + + If ($errorCode#0) + $success:=False + End if
108-108: MAJOR: Preserve blank lines to maintain accurate line numbers.Using
sk ignore empty stringsremoves blank lines from the method body during parsing, causing all subsequent line numbers to shift. This makes coverage data report incorrect line numbers, breaking the accuracy of coverage reports.Remove the
sk ignore empty stringsflag so blank lines are preserved and line indexes match the original source.Apply this diff:
- $lines:=Split string($code; "\r"; sk ignore empty strings) + $lines:=Split string($code; "\r")
🧹 Nitpick comments (4)
docs/coverage-guide.md (2)
60-60: Consider adding language identifiers to fenced code blocks.Several fenced code blocks lack language identifiers, which improves syntax highlighting and readability. Consider adding
textor the appropriate language identifier to the blocks at lines 60, 79, 89, 118, 128, 139, 195, and 284.Example for line 60:
-``` +```text === Code Coverage Report ===Also applies to: 79-79, 89-89, 118-118, 128-128, 139-139, 195-195, 284-284
647-647: Track threshold automation task.The TODO comment notes that automated threshold checking is planned but not yet implemented. This would be a valuable enhancement for CI/CD workflows.
Do you want me to open an issue to track this enhancement or help design the threshold checking implementation?
COVERAGE.md (1)
105-105: Consider adding language identifier to fenced code block.The data flow diagram at line 105 would benefit from a language identifier (e.g.,
textormermaidif it's meant to be a diagram).-``` +```text 1. User enables coverage via parametersCOVERAGE_IMPLEMENTATION.md (1)
60-60: Consider adding language identifiers to fenced code blocks.Multiple fenced code blocks lack language identifiers at lines 60, 79, 89, 118, 128, 139, 195, and 284. Adding identifiers like
text,bash, oryamlwould improve readability.Also applies to: 79-79, 89-89, 118-118, 128-128, 139-139, 195-195, 284-284
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
COVERAGE.md(1 hunks)COVERAGE_IMPLEMENTATION.md(1 hunks)docs/coverage-guide.md(1 hunks)testing/Project/Sources/Classes/CodeInstrumenter.4dm(1 hunks)testing/Project/Sources/Classes/CoverageTracker.4dm(1 hunks)testing/Project/Sources/Methods/CoverageRecordLine.4dm(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- testing/Project/Sources/Classes/CoverageTracker.4dm
🧰 Additional context used
📓 Path-based instructions (1)
testing/Project/Sources/Classes/**/*.4dm
📄 CodeRabbit inference engine (CLAUDE.md)
testing/Project/Sources/Classes/**/*.4dm: Name 4D test classes with the suffix "Test" for auto discovery (e.g., ExampleTest.4dm)
Name test methods with the prefix "test_" so the framework recognizes and runs them
Use comment-based tagging in tests with "// #tags: tag1, tag2" to enable filtering
Opt out of parallel execution for a suite by adding "// #parallel: false" at the test class level
Disable automatic transactions for a test by adding "// #transaction: false" and manage transactions manually if needed
When manual control is required, use Testing context helpers: startTransaction, inTransaction, validateTransaction, cancelTransaction, withTransaction, withTransactionValidate
Control trigger behavior per test with annotations: "// #triggers: enabled" or "// #triggers: disabled"
Place 4D test classes under testing/Project/Sources/Classes/ so the framework can discover and run them
Tag integration tests explicitly with "// #tags: integration" (and enable triggers when needed) to support filtering
Files:
testing/Project/Sources/Classes/CodeInstrumenter.4dm
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: KyleKincer/testing PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T13:15:15.427Z
Learning: Applies to testing/Project/Sources/Classes/*Test.4dm : Use comment-based tagging with lines like "// #tags: unit, integration, slow" to categorize tests
🪛 markdownlint-cli2 (0.18.1)
COVERAGE_IMPLEMENTATION.md
60-60: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
79-79: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
89-89: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
118-118: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
128-128: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
139-139: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
195-195: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
284-284: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/coverage-guide.md
60-60: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
79-79: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
89-89: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
118-118: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
128-128: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
139-139: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
195-195: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
284-284: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
COVERAGE.md
105-105: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (5)
docs/coverage-guide.md (1)
1-692: Excellent comprehensive documentation!This coverage guide is thorough, well-structured, and covers all essential aspects: quick start, technical details, configuration, multiple report formats, best practices, CI/CD integration, troubleshooting, and example workflows. The documentation will enable users to effectively leverage the coverage system.
COVERAGE.md (1)
1-328: Excellent technical documentation!This implementation summary provides clear, detailed technical documentation covering architecture, design decisions, performance considerations, and integration points. The rationale for key decisions is well-articulated and the document serves as a valuable reference for both users and developers.
testing/Project/Sources/Methods/CoverageRecordLine.4dm (1)
1-22: LGTM! Thread-safe coverage recording implementation.The nested
Useblocks correctly implement thread-safe access to shared storage for coverage tracking. The code properly:
- Guards against uninitialized storage
- Creates per-method tracking objects as needed
- Safely increments line execution counters
- Converts line numbers to string keys for 4D object properties
The implementation is concise and correct for 4D's shared storage model.
COVERAGE_IMPLEMENTATION.md (1)
1-384: Comprehensive implementation summary!This completion summary effectively documents the full scope of the coverage implementation, including detailed statistics, deliverables, usage examples, and design decisions. It provides an excellent high-level overview of the feature.
testing/Project/Sources/Classes/CodeInstrumenter.4dm (1)
148-205: Robust executable line detection logic.The
_isExecutableLinefunction correctly identifies which lines should be instrumented by filtering out comments, blank lines, declarations, and non-executable control structure keywords. The logic is comprehensive and handles edge cases well.
Co-authored-by: kyle_kincer <kyle_kincer@sweetwater.com>
Co-authored-by: kyle_kincer <kyle_kincer@sweetwater.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
testing/Project/Sources/Classes/CodeInstrumenter.4dm (1)
106-106: Preserve blank lines when instrumentingUsing
sk ignore empty stringsdrops blank lines from the method body during instrumentation, so every counter after a blank line is assigned to the wrong physical line number. The resulting coverage data misreports which lines executed. Keep the empty entries so line indexes match the original source.- $lines:=Split string($code; "\r") + $lines:=Split string($code; "\r")
🧹 Nitpick comments (1)
testing/Project/Sources/Classes/CodeInstrumenter.4dm (1)
225-240: Consider using built-in trim if availableThe manual trim implementation is correct but could potentially be replaced with 4D's built-in
Trimcommand if available in your target version, reducing code maintenance.If
Trimis available:Function _trim($text : Text) : Text // Trim leading and trailing whitespace - var $result : Text - $result:=$text - - // Trim leading - While (Length($result)>0) && ((Substring($result; 1; 1)=" ") || (Substring($result; 1; 1)=Char(Tab))) - $result:=Substring($result; 2) - End while - - // Trim trailing - While (Length($result)>0) && ((Substring($result; Length($result); 1)=" ") || (Substring($result; Length($result); 1)=Char(Tab))) - $result:=Substring($result; 1; Length($result)-1) - End while - - return $result + return Trim($text)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
testing/Project/Sources/Classes/CodeInstrumenter.4dm(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
testing/Project/Sources/Classes/**/*.4dm
📄 CodeRabbit inference engine (CLAUDE.md)
testing/Project/Sources/Classes/**/*.4dm: Name 4D test classes with the suffix "Test" for auto discovery (e.g., ExampleTest.4dm)
Name test methods with the prefix "test_" so the framework recognizes and runs them
Use comment-based tagging in tests with "// #tags: tag1, tag2" to enable filtering
Opt out of parallel execution for a suite by adding "// #parallel: false" at the test class level
Disable automatic transactions for a test by adding "// #transaction: false" and manage transactions manually if needed
When manual control is required, use Testing context helpers: startTransaction, inTransaction, validateTransaction, cancelTransaction, withTransaction, withTransactionValidate
Control trigger behavior per test with annotations: "// #triggers: enabled" or "// #triggers: disabled"
Place 4D test classes under testing/Project/Sources/Classes/ so the framework can discover and run them
Tag integration tests explicitly with "// #tags: integration" (and enable triggers when needed) to support filtering
Files:
testing/Project/Sources/Classes/CodeInstrumenter.4dm
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: KyleKincer/testing PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T13:15:15.427Z
Learning: Applies to testing/Project/Sources/Classes/*Test.4dm : Use comment-based tagging with lines like "// #tags: unit, integration, slow" to categorize tests
🔇 Additional comments (6)
testing/Project/Sources/Classes/CodeInstrumenter.4dm (6)
9-13: LGTM!Constructor properly initializes all properties with appropriate default values.
15-44: Verify duplicate instrumentation protection.The method doesn't check whether
$methodPathis already inThis.instrumentedMethodsbefore re-instrumenting. IfinstrumentMethodis called twice for the same method, the second call will retrieve already-instrumented code, re-instrument it (injecting duplicate tracking calls), and corrupt the coverage data.Consider adding a guard at the beginning:
Function instrumentMethod($methodPath : Text) : Boolean // Instrument a single method with coverage tracking // Returns true if successful + // Skip if already instrumented + If (This.instrumentedMethods.indexOf($methodPath)#-1) + return True + End if + var $code : Text var $errorCode : Integer
46-75: LGTM!Batch instrumentation logic properly aggregates success/failure statistics and provides useful diagnostics.
77-99: LGTM!The previous review concern about skipping empty original code has been addressed. The method now calls
METHOD SET CODEunconditionally for all instrumented methods and properly clears tracking data after restoration.
146-203: LGTM!Comprehensive coverage of non-executable constructs including comments, declarations, and control structure endings. The logic correctly identifies which lines should be instrumented.
242-248: LGTM!Public getters are implemented correctly.
getInstrumentedMethodPathsdefensively returns a copy to prevent external mutation.
| If (Position("/*"; $line)>0) | ||
| $inMultilineComment:=True | ||
| End if | ||
|
|
||
| If (This._isExecutableLine($line; $inMultilineComment)) | ||
| // Inject coverage tracking before executable line | ||
| var $indent : Text | ||
| $indent:=This._getLineIndentation($line) | ||
|
|
||
| var $trackingCall : Text | ||
| $trackingCall:=$indent+"CoverageRecordLine(\""+$methodPath+"\"; "+String($lineNumber+1)+")" | ||
|
|
||
| $instrumentedLines.push($trackingCall) | ||
| End if | ||
|
|
||
| // Add original line | ||
| $instrumentedLines.push($line) | ||
|
|
||
| // End multiline comment tracking | ||
| If (Position("*/"; $line)>0) | ||
| $inMultilineComment:=False | ||
| End if |
There was a problem hiding this comment.
Fix multiline comment detection for mixed code/comment lines
The current logic sets $inMultilineComment as soon as /* appears anywhere in a line, which prevents instrumentation of executable code that precedes or follows the comment delimiter. For example, var x : Integer /* comment will not be instrumented because $inMultilineComment is set to True before _isExecutableLine is called. This causes coverage gaps for lines with trailing or inline comments.
The fix requires:
- Detecting whether
/*and*/appear on the same line (single-line block comment) - Only setting the multiline flag if
/*is present without a matching*/ - Checking the flag state before examining the current line
Apply this diff:
For ($lineNumber; 0; $lines.length-1)
$line:=$lines[$lineNumber]
- // Track multiline comments (/* ... */)
- If (Position("/*"; $line)>0)
- $inMultilineComment:=True
- End if
-
If (This._isExecutableLine($line; $inMultilineComment))
// Inject coverage tracking before executable line
var $indent : Text
$indent:=This._getLineIndentation($line)
var $trackingCall : Text
$trackingCall:=$indent+"CoverageRecordLine(\""+$methodPath+"\"; "+String($lineNumber+1)+")"
$instrumentedLines.push($trackingCall)
End if
// Add original line
$instrumentedLines.push($line)
- // End multiline comment tracking
- If (Position("*/"; $line)>0)
+ // Track multiline comments (/* ... */)
+ var $openPos; $closePos : Integer
+ $openPos:=Position("/*"; $line)
+ $closePos:=Position("*/"; $line)
+
+ If ($openPos>0) && ($closePos=0)
+ // Start of multiline comment without closing
+ $inMultilineComment:=True
+ Else if ($closePos>0) && ($openPos=0)
+ // End of multiline comment
$inMultilineComment:=False
+ Else if ($openPos>0) && ($closePos>0) && ($closePos<$openPos)
+ // Closing before opening on same line (edge case)
+ $inMultilineComment:=False
End if
End for
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
testing/Project/Sources/Classes/CodeInstrumenter.4dm (2)
119-141: Fix multiline comment detection timing.The multiline comment flag is set (line 120) before calling
_isExecutableLine(line 124), which incorrectly skips lines with trailing or inline comments. For example,var x : Integer /* commentwon't be instrumented because$inMultilineCommentis set toTruebefore the executability check.Additionally, single-line block comments like
/* comment */are not handled correctly—the flag is set and cleared in the same iteration, but the order causes the line to be treated as inside a multiline comment.Apply the fix from the previous review to check comment boundaries after the executability test:
For ($lineNumber; 0; $lines.length-1) $line:=$lines[$lineNumber] - // Track multiline comments (/* ... */) - If (Position("/*"; $line)>0) - $inMultilineComment:=True - End if - If (This._isExecutableLine($line; $inMultilineComment)) // Inject coverage tracking before executable line var $indent : Text $indent:=This._getLineIndentation($line) var $trackingCall : Text $trackingCall:=$indent+"CoverageRecordLine(\""+$methodPath+"\"; "+String($lineNumber+1)+")" $instrumentedLines.push($trackingCall) End if // Add original line $instrumentedLines.push($line) - // End multiline comment tracking - If (Position("*/"; $line)>0) + // Track multiline comments (/* ... */) + var $openPos; $closePos : Integer + $openPos:=Position("/*"; $line) + $closePos:=Position("*/"; $line) + + If ($openPos>0) && ($closePos=0) + // Start of multiline comment without closing + $inMultilineComment:=True + Else if ($closePos>0) && ($openPos=0) + // End of multiline comment $inMultilineComment:=False + Else if ($openPos>0) && ($closePos>0) && ($closePos<$openPos) + // Closing before opening on same line (edge case) + $inMultilineComment:=False End if End for
130-130: Escape method path to prevent syntax errors.The method path is directly concatenated into a string literal without escaping. If
$methodPathcontains quotes or backslashes (e.g.,MyClass["property"]), the generated tracking call will have syntax errors and instrumentation will fail.Apply this diff to escape the path:
var $trackingCall : Text - $trackingCall:=$indent+"CoverageRecordLine(\""+$methodPath+"\"; "+String($lineNumber+1)+")" + var $escapedPath : Text + $escapedPath:=Replace string($methodPath; "\\"; "\\\\") + $escapedPath:=Replace string($escapedPath; "\""; "\\\"") + $trackingCall:=$indent+"CoverageRecordLine(\""+$escapedPath+"\"; "+String($lineNumber+1)+")"
🧹 Nitpick comments (1)
testing/Project/Sources/Classes/CodeInstrumenter.4dm (1)
146-203: Consider expanding non-executable line detection.The current heuristics skip common non-executable lines, but several additional patterns could be skipped:
Case of(line 185 checks ending, but not opening)Else if(condition evaluates, but structure keyword)Usewithout pairedEnd usecheck- Loop declarations:
For,For each,While(structure keywords, not the loop body)Endwithout specific keyword (rare but possible)Expanding these checks would reduce false positives where structural keywords are instrumented unnecessarily.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
testing/Project/Sources/Classes/CodeInstrumenter.4dm(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
testing/Project/Sources/Classes/**/*.4dm
📄 CodeRabbit inference engine (CLAUDE.md)
testing/Project/Sources/Classes/**/*.4dm: Name 4D test classes with the suffix "Test" for auto discovery (e.g., ExampleTest.4dm)
Name test methods with the prefix "test_" so the framework recognizes and runs them
Use comment-based tagging in tests with "// #tags: tag1, tag2" to enable filtering
Opt out of parallel execution for a suite by adding "// #parallel: false" at the test class level
Disable automatic transactions for a test by adding "// #transaction: false" and manage transactions manually if needed
When manual control is required, use Testing context helpers: startTransaction, inTransaction, validateTransaction, cancelTransaction, withTransaction, withTransactionValidate
Control trigger behavior per test with annotations: "// #triggers: enabled" or "// #triggers: disabled"
Place 4D test classes under testing/Project/Sources/Classes/ so the framework can discover and run them
Tag integration tests explicitly with "// #tags: integration" (and enable triggers when needed) to support filtering
Files:
testing/Project/Sources/Classes/CodeInstrumenter.4dm
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: KyleKincer/testing PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T13:15:15.427Z
Learning: Applies to testing/Project/Sources/Classes/*Test.4dm : Use comment-based tagging with lines like "// #tags: unit, integration, slow" to categorize tests
📚 Learning: 2025-09-22T13:15:15.427Z
Learnt from: CR
Repo: KyleKincer/testing PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T13:15:15.427Z
Learning: Applies to testing/Project/Sources/Classes/*Test.4dm : Use comment-based tagging with lines like "// #tags: unit, integration, slow" to categorize tests
Applied to files:
testing/Project/Sources/Classes/CodeInstrumenter.4dm
🔇 Additional comments (5)
testing/Project/Sources/Classes/CodeInstrumenter.4dm (5)
1-13: LGTM!Properties are well-defined and the constructor properly initializes all state. Note that
hostStorageis stored but not referenced elsewhere in this class, andcoverageTrackerremainsNullunless set externally.
46-75: LGTM!Batch instrumentation logic is clean and provides useful statistics for tracking instrumentation results.
77-99: LGTM!The restoration logic correctly handles all methods including those with empty original code. The fix from previous review has been successfully applied.
205-223: LGTM!Indentation extraction correctly handles both spaces and tabs, preserving the original formatting for injected tracking calls.
225-248: LGTM!The trim utility and getter methods are correctly implemented. Returning a copy of
instrumentedMethodsin the getter is good defensive programming.
| Function instrumentMethod($methodPath : Text) : Boolean | ||
| // Instrument a single method with coverage tracking | ||
| // Returns true if successful | ||
|
|
||
| var $code : Text | ||
| var $errorCode : Integer | ||
|
|
||
| // Get original code | ||
| METHOD GET CODE($methodPath; $code; *; $errorCode) | ||
|
|
||
| If ($errorCode#0) || ($code="") | ||
| return False | ||
| End if | ||
|
|
||
| // Store original code for restoration | ||
| This.originalCode[$methodPath]:=$code | ||
|
|
||
| // Instrument the code | ||
| var $instrumentedCode : Text | ||
| $instrumentedCode:=This._instrumentCodeLines($code; $methodPath) | ||
|
|
||
| // Set instrumented code | ||
| METHOD SET CODE($methodPath; $instrumentedCode; *; $errorCode) | ||
|
|
||
| If ($errorCode#0) | ||
| return False | ||
| End if | ||
|
|
||
| This.instrumentedMethods.push($methodPath) | ||
| return True |
There was a problem hiding this comment.
Consider instrumenting blank methods for consistency.
Line 25 returns False when $code="", which skips instrumentation of intentionally blank methods. However, restoreOriginalCode now handles empty code (per previous fix). This inconsistency means blank methods are never tracked in instrumentedMethods, but they also don't need restoration. If blank methods are meant to be skipped, the current logic is correct. Otherwise, consider instrumenting them for complete coverage tracking.
gamescoot
left a comment
gamescoot
left a comment
There was a problem hiding this comment.
Just a few concerns. I feel like the amount of documentation seems a bit much. That's the main concern. How long does this take to run?
| @@ -0,0 +1,386 @@ | |||
| # Code Coverage Implementation - Completion Summary | |||
There was a problem hiding this comment.
Do these MD files help ai somehow? If not then they're probably not needed. Just short change to the readme on how to use and what it does.
| tool4d --project YourProject.4DProject --startup-method "test" --user-param "tags=unit excludeTags=slow" | ||
| ``` | ||
|
|
||
| ## Code Coverage |
There was a problem hiding this comment.
This section is really long. If you want it to contain all this info, that's fine, but it makes it feel like this is mostly a code coverage tool given how much of the readme is dedicated to it.
| @@ -0,0 +1,693 @@ | |||
| # Code Coverage Guide | |||
There was a problem hiding this comment.
There's a lot of md files about this stuff. Any way we could consolidate these or just link to the main one when needed?
| End if | ||
|
|
||
| // Store original code for restoration | ||
| This.originalCode[$methodPath]:=$code |
There was a problem hiding this comment.
Curious how this affects application memory usage in large projects.
| // Show uncovered lines if coverage < 100% | ||
| If ($method.coveragePercent<100) | ||
| var $uncoveredLines : Collection | ||
| $uncoveredLines:=This.coverageTracker.getUncoveredLines($method.method) |
There was a problem hiding this comment.
Does this make the report very large for symphony? Maybe we could have an option to not print out all the lines since it might not be helpful until our coverage is better or more targeted?
| : ($percent>=50) | ||
| return "moderate" | ||
| Else | ||
| return "poor" |
There was a problem hiding this comment.
<10
return "We're trying our best, ok? Gotta start somewhere..."
| $css:=$css+".coverage-excellent { background: #27ae60; color: white; }\r\n" | ||
| $css:=$css+".coverage-good { background: #f39c12; color: white; }\r\n" | ||
| $css:=$css+".coverage-moderate { background: #e67e22; color: white; }\r\n" | ||
| $css:=$css+".coverage-poor { background: #e74c3c; color: white; }\r\n" |
There was a problem hiding this comment.
We're not gonna see orange for a long time.
3 participants
Add a comprehensive code coverage system to the 4D Unit Testing Framework to enable coverage metrics for host project tests.
The 4D platform lacks native code coverage hooks, necessitating a custom solution. This PR implements runtime code instrumentation using
METHOD GET CODEandMETHOD SET CODEto inject execution counters, collect line-level coverage data, and generate reports in various formats (text, JSON, HTML, LCOV). This allows for detailed coverage analysis and seamless integration with CI/CD pipelines.Summary by CodeRabbit
New Features
Tooling
Documentation
Tests