Skip to content

Conversation

karim-agha
Copy link
Contributor

#36

Migrate the rest of the test to the new test utility introduced in #606 ---

This PR introduces the following changes:

  • Migrates all non-flashblocks integration tests to the new test harness framework
  • Fixes all build warnings
  • Adds few helper functions and types for testing
  • Few refactorings of the test infrastructure
  • Refactored the way the tester binary is linking to op-rbuilder dependencies
  • Moved all tests under tests/ directory
  • Removed integration directory`

now running cargo test will output

$ cargo test
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.36s
     Running unittests src/lib.rs (target/debug/deps/op_rbuilder-7bb461ec2888ee4b)

running 8 tests
test tx_signer::test::test_sign_transaction ... ok
test tests::vanilla::smoke::get_payload_close_to_fcu ... ok
test tests::vanilla::smoke::transaction_flood_no_sleep ... ok
test tests::vanilla::ordering::fee_priority_ordering ... ok
test tests::vanilla::revert::monitor_transaction_drops ... ok
test tests::vanilla::revert::revert_protection ... ok
test tests::vanilla::revert::revert_protection_disabled ... ok
test tests::vanilla::smoke::chain_produces_blocks ... ok

test result: ok. 8 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 23.67s

     Running unittests src/main.rs (target/debug/deps/op_rbuilder-87f69711ab7f0a5e)

running 7 tests
test generator::tests::test_block_cell_update_value ... ok
test generator::tests::test_block_cell_immediate_value ... ok
test generator::tests::test_job_deadline ... ok
test tx_signer::test::test_sign_transaction ... ok
test generator::tests::test_block_cell_wait_for_value ... ok
test generator::tests::test_block_cell_multiple_waiters ... ok
test generator::tests::test_payload_generator ... ok

test result: ok. 7 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 4.70s

   Doc-tests op_rbuilder

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

📝 Summary

💡 Motivation and Context


✅ I have completed the following steps:

  • Run make lint
  • Run make test
  • Added tests (if applicable)

#36

Migrate the rest of the test to the new test utility introduced in #606
---

This PR introduces the following changes:
- Migrates all non-flashblocks integration tests to the new test harness framework
- Fixes all build warnings
- Adds few helper functions and types for testing
- Few refactorings of the test infrastructure
- Refactored the way the `tester` binary is linking to op-rbuilder dependencies
- Moved all tests under `tests/` directory
- Removed `integration` directory`

now running `cargo test` will output

```
$ cargo test
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.36s
     Running unittests src/lib.rs (target/debug/deps/op_rbuilder-7bb461ec2888ee4b)

running 8 tests
test tx_signer::test::test_sign_transaction ... ok
test tests::vanilla::smoke::get_payload_close_to_fcu ... ok
test tests::vanilla::smoke::transaction_flood_no_sleep ... ok
test tests::vanilla::ordering::fee_priority_ordering ... ok
test tests::vanilla::revert::monitor_transaction_drops ... ok
test tests::vanilla::revert::revert_protection ... ok
test tests::vanilla::revert::revert_protection_disabled ... ok
test tests::vanilla::smoke::chain_produces_blocks ... ok

test result: ok. 8 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 23.67s

     Running unittests src/main.rs (target/debug/deps/op_rbuilder-87f69711ab7f0a5e)

running 7 tests
test generator::tests::test_block_cell_update_value ... ok
test generator::tests::test_block_cell_immediate_value ... ok
test generator::tests::test_job_deadline ... ok
test tx_signer::test::test_sign_transaction ... ok
test generator::tests::test_block_cell_wait_for_value ... ok
test generator::tests::test_block_cell_multiple_waiters ... ok
test generator::tests::test_payload_generator ... ok

test result: ok. 7 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 4.70s

   Doc-tests op_rbuilder

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

```
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR migrates existing integration tests to the new test harness framework, restructures the tests/ directory, and updates project configuration to remove the old integration folder while fixing build warnings.

  • Moves all non-flashblocks integration tests under tests/vanilla using TestHarnessBuilder
  • Refactors test framework modules and removes duplicated integration code
  • Updates Cargo.toml, main.rs, and lib.rs to reflect the new test setup and dependencies

Reviewed Changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.

File Description
crates/op-rbuilder/src/tests/vanilla/revert.rs Updated revert tests to use new harness; minor typos in comments
crates/op-rbuilder/src/tests/vanilla/ordering.rs Added fee priority ordering test under new harness
crates/op-rbuilder/src/generator.rs Adjusted RNG import in unit test
crates/op-rbuilder/Cargo.toml Reorganized test-related dependencies and binary configuration
Comments suppressed due to low confidence (3)

crates/op-rbuilder/src/tests/vanilla/revert.rs:4

  • Fix typo in comment: change 'an' to 'and' (or 'are') so it reads 'transactions that get reverted and not included in the block'.
/// This test ensures that the transactions that get reverted an not included in the block

crates/op-rbuilder/src/generator.rs:439

  • The import rand::rng and call rng() are invalid; use use rand::thread_rng; and initialize with thread_rng() or another valid RNG function.
use rand::rng;

crates/op-rbuilder/Cargo.toml:141

  • The [[bin]] section for the tester binary is missing a path attribute; add path = "src/tester/main.rs" so Cargo can locate the source file.
required-features = ["tests"]

Copy link
Contributor

@ferranbt ferranbt left a comment

Choose a reason for hiding this comment

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

This PR significantly exceeds the scope of issue #36 which was intended to simply migrate remaining tests to use the existing test utility from PR #606.

I do agree with some of the architectural improvements, but I suggest splitting this work into smaller more targeted PRs. It would make review more manageable and more focused discussion on some of the changes.

- `make tester` now builds
- Ability to run GH Actions locally through `act`
@karim-agha karim-agha merged commit 77df514 into main May 20, 2025
3 checks passed
@karim-agha karim-agha deleted the karim/tests-migration branch May 20, 2025 09:14
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.

3 participants