Skip to content

test(vm): add comprehensive unit tests for bytecode argument encoding and decoding (#4240)#4735

Open
ParthMozarkar wants to merge 2 commits intoboa-dev:mainfrom
ParthMozarkar:fix-4240
Open

test(vm): add comprehensive unit tests for bytecode argument encoding and decoding (#4240)#4735
ParthMozarkar wants to merge 2 commits intoboa-dev:mainfrom
ParthMozarkar:fix-4240

Conversation

@ParthMozarkar
Copy link

Summary

This PR adds comprehensive unit tests for bytecode argument encoding and decoding in:

core/engine/src/vm/opcode/args.rs

as requested in issue #4240.

What’s Included

  • ✅ Tests verifying correct encoding and decoding of opcode arguments.
  • ✅ Round-trip validation (encode → decode → assert equality).
  • ✅ Boundary condition tests for edge-case values.
  • ✅ Out-of-bounds read tests to ensure panics occur as expected.
  • ✅ Negative test cases validating safety guarantees.

Motivation

Bytecode argument handling is a critical component of the VM execution pipeline.
These tests strengthen confidence in argument parsing correctness and protect against silent corruption or unsafe memory access.

By covering both valid and invalid cases, this improves robustness and makes future refactors safer.

Verification

  • cargo test passes successfully.
  • No Clippy warnings introduced.
  • Formatting verified with cargo fmt.

Closes #4240

@ParthMozarkar ParthMozarkar requested a review from a team as a code owner February 26, 2026 07:49
@github-actions
Copy link

github-actions bot commented Feb 26, 2026

Test262 conformance changes

Test result main count PR count difference
Total 52,963 52,963 0
Passed 49,935 49,935 0
Ignored 2,207 2,207 0
Failed 821 821 0
Panics 0 0 0
Conformance 94.28% 94.28% 0.00%

Tested main commit: 5acb570038007c606d75b17bed6ee9a02117ce97
Tested PR commit: adabe45a971eb2e48bf9b96ec350ffbd54d8c465
Compare commits: 5acb570...adabe45

@Sharktheone
Copy link

Sharktheone commented Feb 26, 2026

I think you should base this off main and not off your own branch with console.table implemented

@ParthMozarkar
Copy link
Author

Thanks for pointing that out you're right.

I’ve rebased the branch on top of main and force pushed the updated changes so the PR now only contains the unit test additions for #4240.

Please let me know if anything else needs adjustment. 🙌

@jedel1043 jedel1043 added C-Tests Issues and PRs related to the tests. C-VM Issues and PRs related to the Boa Virtual Machine. waiting-on-review labels Feb 27, 2026
@ParthMozarkar
Copy link
Author

@jedel1043
everything is perfect i feel like so if you could please review it for merging...it would be great !!

Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Thanks, looks great!

@jedel1043
Copy link
Member

You might need to merge pull the main branch btw

@codecov
Copy link

codecov bot commented Mar 5, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.17%. Comparing base (6ddc2b4) to head (973ec58).
⚠️ Report is 841 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #4735       +/-   ##
===========================================
+ Coverage   47.24%   59.17%   +11.93%     
===========================================
  Files         476      563       +87     
  Lines       46892    62363    +15471     
===========================================
+ Hits        22154    36905    +14751     
- Misses      24738    25458      +720     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ParthMozarkar
Copy link
Author

You might need to merge pull the main branch btw

Hey,
thanks for the suggestion...i have merged into the main branch so pleease review it !!

@jedel1043 jedel1043 enabled auto-merge March 6, 2026 07:27
@ParthMozarkar
Copy link
Author

Hey, could you just approve the last 3 workflows awaiting so we can then merge this pr and work upon another?

auto-merge was automatically disabled March 6, 2026 11:12

Head branch was pushed to by a user without write access

@jedel1043 jedel1043 enabled auto-merge March 6, 2026 23:32
auto-merge was automatically disabled March 7, 2026 12:28

Head branch was pushed to by a user without write access

@jedel1043 jedel1043 added Waiting On Author Waiting on PR changes from the author and removed waiting-on-review labels Mar 11, 2026
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Where are all the tests? You deleted them or something

… and decoding

This commit restores the missing unit tests in args.rs and includes the approved audit.toml changes, while removing unrelated console.table changes from the PR.
@ParthMozarkar
Copy link
Author

Where are all the tests? You deleted them or something

so sorry i somehow replaced it or something....but now i have fixed everything....the errors of CI and also brought back the tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-Tests Issues and PRs related to the tests. C-VM Issues and PRs related to the Boa Virtual Machine. Waiting On Author Waiting on PR changes from the author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add unit tests for bytecode arguments reading

3 participants