Skip to content

[T-004] Add comprehensive test suite for flux-js VM#1

Open
SuperInstance wants to merge 1 commit intomainfrom
greenhorn/T-004-flux-js
Open

[T-004] Add comprehensive test suite for flux-js VM#1
SuperInstance wants to merge 1 commit intomainfrom
greenhorn/T-004-flux-js

Conversation

@SuperInstance
Copy link
Copy Markdown
Owner

@SuperInstance SuperInstance commented Apr 12, 2026

Summary

  • Added Vitest test framework (previously 0 tests)
  • Created 6 test files with 161 tests covering all modules
  • Fixed 3 source bugs discovered during testing
  • All 161 tests passing in 763ms

Bug Fixes

BUG 1: JMP opcode compound assignment (V8 unsafe)

this.pc += this._i16() reads this.pc before calling this._i16(), but _i16() modifies this.pc. V8 caches the stale value, applying the offset to the wrong base.

Fix: Store offset in variable first.

BUG 2: JMP label resolution off by 2

BUG 3: JNZ/JZ label resolution off by 1

Part of T-004


Staging: Open in Devin

- Added Vitest framework (previously 0 tests)
- Created 6 test files with 161 tests covering:
  - VM core: 45 tests (all opcodes, registers, stack, jumps, loops, errors)
  - Assembler: 32 tests (encoding, labels, comments, case insensitivity)
  - Disassembler: 22 tests (all instruction formats, round-trip)
  - Vocabulary/Interpreter: 37 tests (10 NL patterns, result format)
  - A2A/Swarm: 21 tests (agents, messaging, consensus, integration)
  - Opcodes: 4 tests (constants verification)
- Fixed 3 source bugs found during testing:
  - BUG 1: JMP opcode used compound assignment this.pc+=this._i16()
    which is unsafe in V8 — _i16() modifies this.pc before the
    compound assignment reads it, causing incorrect offset base
  - BUG 2: Assembler label resolution for JMP was off by 2
    (used bc.length instead of bc.length + 2 as currentPc)
  - BUG 3: Assembler label resolution for JNZ/JZ was off by 1
    (used bc.length + 1 instead of bc.length + 2)
- All 161 tests passing
Copy link
Copy Markdown

@beta-devin-ai-integration beta-devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

View 4 additional findings in Devin Review.

Staging: Open in Devin

Comment thread flux.js
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Incomplete fix: MOVI label resolution has same off-by-one that was fixed in JNZ/JZ

The PR correctly fixes the resolveValue currentPc offset for JNZ/JZ (line 179: bc.length + 1bc.length + 2) and JMP (line 183: bc.lengthbc.length + 2), but the identical pattern in MOVI (line 175) was left unfixed at bc.length + 1. MOVI has the same 4-byte format as JNZ/JZ: after bc.push(op, register), bc.length = S + 2. The VM reads MOVI as opcode(1) + _u8()(1) + _i16()(2) = 4 bytes, so PC ends at S + 4. The correct currentPc should be bc.length + 2 = S + 4, but the code passes bc.length + 1 = S + 3, producing an off-by-one error when a label is used as the MOVI operand (e.g., MOVI R0, some_label).

(Refers to line 175)

Staging: Open in Devin

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

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.

1 participant