test: lift new-code coverage on Disassembler, GrobValue, GrobStruct, …#41
Merged
Conversation
…GrobFunction Sonar new-code coverage was 64.2%, below the 80% gate. Disassembler: data-driven theory covers every OpCode dispatch arm, asserting the dispatch returns the documented instruction width and emits the opcode name. Width table mirrors the production dispatch so any future width drift fails loudly here. GrobValue: per-kind Equals/GetHashCode tests for Map and Function reference paths, +0.0/-0.0 hash agreement, full strict and try-accessor matrices, and ArgumentNullException tests for the five reference-typed From* factories. GrobStruct: null/empty TypeName guards, null field-name guards on GetField/SetField/TryGetField, missing-key behaviour, reflexive Equals and field-equality coverage. GrobFunction: null name, negative arity, and empty-name-allowed contract.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds per-byte source column tracking and runtime exception types, implements a fixed-capacity ValueStack, implements a sealed VirtualMachine dispatch loop (arithmetic, constants, Print, error mapping and DEBUG tracing), and adds comprehensive VM and Chunk tests. ChangesBytecode VM Engine
Sequence DiagramsequenceDiagram
participant Caller
participant VirtualMachine
participant ValueStack
participant Output
Caller->>VirtualMachine: Run(Chunk)
loop Each instruction
VirtualMachine->>VirtualMachine: Fetch opcode, line, column
alt Constant/Push
VirtualMachine->>ValueStack: Push(...)
else Arithmetic
VirtualMachine->>ValueStack: Pop operands
VirtualMachine->>VirtualMachine: Execute checked arithmetic (map to E5001–E5005)
VirtualMachine->>ValueStack: Push result
else Print
VirtualMachine->>ValueStack: Peek top
VirtualMachine->>Output: Write formatted value
end
end
VirtualMachine->>Caller: Return or throw runtime/internal exception
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
tests/Grob.Vm.Tests/VirtualMachineTests.cs (2)
1-3: ⚡ Quick winReorder
usingdirectives to match repository convention.
Xunitshould come beforeGrob.Core(System.*, external, thenGrob.*).Proposed fix
using System.Text; -using Grob.Core; using Xunit; +using Grob.Core;As per coding guidelines "Order
usingdirectives as:System.*first, then external packages, thenGrob.*, followed by a blank line, then file-scoped namespace declaration".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Grob.Vm.Tests/VirtualMachineTests.cs` around lines 1 - 3, Reorder the using directives at the top of VirtualMachineTests.cs so they follow the repo convention: System.* first (e.g., System.Text), then external packages (e.g., Xunit), then project namespaces (e.g., Grob.Core); ensure a blank line follows the using block before the file-scoped namespace declaration (if present). Locate the using lines containing System.Text, Grob.Core, and Xunit and reorder them accordingly and add the blank line after the grouped usings.
83-83: ⚡ Quick winNormalise test method names to a single underscore shape.
Several methods use more than one underscore. The test naming contract here is two PascalCase segments joined by exactly one underscore.
As per coding guidelines "Test method names must use the shape
Subject_BehaviourClause— two PascalCase segments joined by a single underscore".Also applies to: 122-122, 191-191, 208-208, 223-223, 343-343, 355-355, 467-467
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Grob.Vm.Tests/VirtualMachineTests.cs` at line 83, The test method names use multiple underscores; rename them to the required Subject_BehaviourClause form with exactly one underscore and two PascalCase segments: rename IntArithmetic_BinaryOps_ProduceExpectedResult to IntArithmetic_BinaryOpsProduceExpectedResult (i.e., Subject = IntArithmetic, Behaviour = BinaryOpsProduceExpectedResult) and apply the same transformation to the other test methods referenced (those with multiple underscores) so each becomes OneSubjectSegment_OnePascalCaseBehaviourSegment; update all references/usages accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Grob.Core/GrobRuntimeException.cs`:
- Around line 33-41: Move the public sealed class GrobArithmeticException out of
the file containing GrobRuntimeException and into its own file named
GrobArithmeticException.cs; keep the same namespace, XML doc comment,
constructor signature (public GrobArithmeticException(string code, int line,
string message) : base(code, line, message) { }), and accessibility, delete the
original type from the GrobRuntimeException file, and ensure the new file
compiles (add any required usings and update the project if necessary).
In `@src/Grob.Vm/ValueStack.cs`:
- Around line 55-59: The Peek method currently computes index = _top - 1 -
distance without validating distance, so a negative distance can produce an
out-of-range index; add an explicit guard at the start of ValueStack.Peek to
throw a GrobInternalException when distance < 0 (or otherwise reject negative
distances) before computing index, keeping the existing underflow check for
index < 0 and referencing _top and _values in the error handling to avoid raw
IndexOutOfRangeException.
In `@src/Grob.Vm/VirtualMachine.cs`:
- Around line 69-77: The opcode cases in VirtualMachine (cases OpCode.Constant,
OpCode.ConstantLong and the other multi-byte operand cases) currently call
Chunk.ReadByte(ip/ ip+1) without verifying remaining bytes; add explicit bounds
checks against the chunk's remaining byte count (using chunk.Count or similar)
before fetching operands and update ip only after validation, and if
insufficient bytes throw a VM-level internal error (not
ArgumentOutOfRangeException) so the VM reports a controlled error; specifically
guard reads in the Constant, ConstantLong (and the case around lines 85–87)
paths that use Chunk.ReadByte and Chunk.ReadConstant.
- Around line 46-50: Run currently leaves the VM's instance operand stack
(_stack) intact between invocations, which can leak values when reusing
VirtualMachine; at the start of the Run(Chunk chunk) method clear or
reinitialize the operand stack (e.g., call _stack.Clear() or assign a new stack)
and also reset any stack pointer/stackTop fields used by the VM so the VM starts
with an empty operand stack for each chunk.
In `@tests/Grob.Vm.Tests/VirtualMachineTests.cs`:
- Around line 240-252: The test IntModuloByZero_ThrowsE5003 only asserts
ex.Code; update it (and the other arithmetic error tests at the referenced
ranges) to assert the full diagnostic contract by also asserting ex.Line and
ex.Column (use the 1-based positions passed into Chunk.WriteOpCode in this test
— e.g. Assert.Equal(1, ex.Line) and Assert.Equal(1, ex.Column)); locate the
checks around GrobArithmeticException (the Assert.Throws call that invokes
vm.Run(chunk)) and add these two assertions immediately after verifying ex.Code.
---
Nitpick comments:
In `@tests/Grob.Vm.Tests/VirtualMachineTests.cs`:
- Around line 1-3: Reorder the using directives at the top of
VirtualMachineTests.cs so they follow the repo convention: System.* first (e.g.,
System.Text), then external packages (e.g., Xunit), then project namespaces
(e.g., Grob.Core); ensure a blank line follows the using block before the
file-scoped namespace declaration (if present). Locate the using lines
containing System.Text, Grob.Core, and Xunit and reorder them accordingly and
add the blank line after the grouped usings.
- Line 83: The test method names use multiple underscores; rename them to the
required Subject_BehaviourClause form with exactly one underscore and two
PascalCase segments: rename IntArithmetic_BinaryOps_ProduceExpectedResult to
IntArithmetic_BinaryOpsProduceExpectedResult (i.e., Subject = IntArithmetic,
Behaviour = BinaryOpsProduceExpectedResult) and apply the same transformation to
the other test methods referenced (those with multiple underscores) so each
becomes OneSubjectSegment_OnePascalCaseBehaviourSegment; update all
references/usages accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f3f71ec-b793-4c82-ac22-a4abc711447b
📒 Files selected for processing (4)
src/Grob.Core/GrobRuntimeException.cssrc/Grob.Vm/ValueStack.cssrc/Grob.Vm/VirtualMachine.cstests/Grob.Vm.Tests/VirtualMachineTests.cs
…plit - Split GrobArithmeticException into its own file (one public type per file). - Guard negative distance in ValueStack.Peek; add internal Reset() to clear stack at Run entry. - VirtualMachine.Run now resets the operand stack on entry so a prior exception-terminated run cannot leak values into a subsequent invocation. - Tests: rename multi-underscore methods to Subject_BehaviourClause shape; add ex.Line assertions to IntModuloByZero/FloatDivideByZero/FloatModuloByZero.
- Chunk now stores a parallel per-byte _columns list; adds GetColumn(offset) reader and column-bearing WriteByte/WriteOpCode overloads. Legacy 2-arg overloads remain and record column=0 (sentinel for 'unknown'). - GrobRuntimeException gains a Column property and a 4-arg ctor; legacy 3-arg ctor forwards with column=0. GrobArithmeticException mirrors both ctors. - VirtualMachine.Run tracks the column alongside the line and threads it into all five arithmetic-error throws (E5001..E5005). #if DEBUG trace is the only path that touches the new column on every iteration; Release reads it only on a throw. - Tests: new ChunkColumnTests round-trips column per byte; IntModuloByZero/FloatDivideByZero/FloatModuloByZero now assert ex.Column end-to-end.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Grob.Vm.Tests/VirtualMachineTests.cs (1)
349-357:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the runtime error code in the overflow test.
This test checks line and stack count but not
ex.Code. A code-mapping regression would pass unnoticed.As per coding guidelines "Every test that exercises error output must assert the full diagnostic contract: exact diagnostic code, 1-based line number, and exact 1-based column number."Proposed fix
var ex = Assert.Throws<GrobRuntimeException>( () => stack.Push(GrobValue.FromInt(0), line: 99)); + Assert.Equal("E5903", ex.Code); Assert.Equal(99, ex.Line); Assert.Equal(ValueStack.Capacity, stack.Count);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Grob.Vm.Tests/VirtualMachineTests.cs` around lines 349 - 357, The test ValueStackOverflow_SurfacesAsRuntimeErrorNotUnguardedWrite must assert the full diagnostic contract: add assertions that the thrown GrobRuntimeException exposes the exact diagnostic code and the 1-based column number in addition to existing line and stack count checks; specifically, in that test assert ex.Code equals the expected value-stack-overflow enum/constant (the project’s value-stack overflow diagnostic code) and assert ex.Column == 1 (the expected 1-based column).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Grob.Vm/ValueStack.cs`:
- Around line 66-70: The XML summary for Reset() is inaccurate: the method does
clear the live region using Array.Clear, so update the comment in ValueStack.cs
to state that Reset() clears the live slots (releasing references) and sets the
top pointer to zero; reference existing members Push() and Pop() in the summary
to clarify that Push will overwrite slots after Reset and Pop already clears
individual slots, ensuring the doc matches the implementation.
---
Outside diff comments:
In `@tests/Grob.Vm.Tests/VirtualMachineTests.cs`:
- Around line 349-357: The test
ValueStackOverflow_SurfacesAsRuntimeErrorNotUnguardedWrite must assert the full
diagnostic contract: add assertions that the thrown GrobRuntimeException exposes
the exact diagnostic code and the 1-based column number in addition to existing
line and stack count checks; specifically, in that test assert ex.Code equals
the expected value-stack-overflow enum/constant (the project’s value-stack
overflow diagnostic code) and assert ex.Column == 1 (the expected 1-based
column).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e0fa4e70-aaf4-4d87-ad2d-17f2c792533e
📒 Files selected for processing (5)
src/Grob.Core/GrobArithmeticException.cssrc/Grob.Core/GrobRuntimeException.cssrc/Grob.Vm/ValueStack.cssrc/Grob.Vm/VirtualMachine.cstests/Grob.Vm.Tests/VirtualMachineTests.cs
💤 Files with no reviewable changes (1)
- src/Grob.Core/GrobRuntimeException.cs
✅ Files skipped from review due to trivial changes (1)
- src/Grob.Core/GrobArithmeticException.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Grob.Core/Chunk.cs`:
- Around line 57-61: In Chunk.WriteByte(byte value, int line, int column) reject
negative column values by validating column >= 0 and throw an ArgumentException
(with a clear parameter name "column") when column < 0; update the method to
perform this guard before mutating _code, _lines, and _columns so no invalid
source metadata is added.
In `@src/Grob.Core/GrobRuntimeException.cs`:
- Around line 41-46: The public constructor GrobRuntimeException(string code,
int line, int column, string message) must validate inputs: throw
ArgumentNullException if code is null, ArgumentException if code is
empty/whitespace, ArgumentOutOfRangeException (or ArgumentException per
guidelines) if line < 1, and ArgumentOutOfRangeException if column < 0; add
these checks at the start of the constructor for Code, Line and Column and use
the specific exception types (ArgumentNullException and
ArgumentException/ArgumentOutOfRangeException) with clear parameter names to
enforce valid diagnostics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 106eb02b-af7b-48ec-a47f-46a1859335e3
📒 Files selected for processing (6)
src/Grob.Core/Chunk.cssrc/Grob.Core/GrobArithmeticException.cssrc/Grob.Core/GrobRuntimeException.cssrc/Grob.Vm/VirtualMachine.cstests/Grob.Core.Tests/ChunkColumnTests.cstests/Grob.Vm.Tests/VirtualMachineTests.cs
- VirtualMachine.Run: SuppressMessage S3776 (cognitive complexity). Bytecode dispatch is a single switch by design per D-302; extracting per-opcode handlers would add a call frame per instruction. - VirtualMachine DivideFloat/ModuloFloat: scoped #pragma warning disable S1244 around the b == 0.0 guards. Exact-zero check is intentional per D-273: +0.0/-0.0 both caught, NaN propagates as NaN, no tolerance bias on small divisors.
- ValueStack.Reset: docstring now matches behaviour (live region IS cleared via Array.Clear so reference slots are released to GC per D-304).
- Chunk.WriteByte(byte,int,int): rejects negative column with ArgumentOutOfRangeException. Columns are 1-based or 0 ('unknown'); negative values are never valid source metadata.
- GrobRuntimeException 4-arg ctor: validates code (non-null, non-empty), line (>= 1), column (>= 0), and message (non-null, non-empty) at the public boundary.
Mirror the validation added to GrobRuntimeException so the contract is enforced at the write boundary instead of only surfacing on a downstream throw. Line is always 1-based and never optional (unlike column, where 0 is a valid 'unknown' sentinel).
|
This was referenced May 30, 2026
This was referenced Jun 19, 2026
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
1 participant
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.
…GrobFunction
Sonar new-code coverage was 64.2%, below the 80% gate.
Disassembler: data-driven theory covers every OpCode dispatch arm, asserting the dispatch returns the documented instruction width and emits the opcode name. Width table mirrors the production dispatch so any future width drift fails loudly here.
GrobValue: per-kind Equals/GetHashCode tests for Map and Function reference paths, +0.0/-0.0 hash agreement, full strict and try-accessor matrices, and ArgumentNullException tests for the five reference-typed From* factories.
GrobStruct: null/empty TypeName guards, null field-name guards on GetField/SetField/TryGetField, missing-key behaviour, reflexive Equals and field-equality coverage.
GrobFunction: null name, negative arity, and empty-name-allowed contract.
Summary by CodeRabbit
New Features
Tests