chore(blockifier): create unit tests for contract_class_manager

[avivg-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[avivg-starkware]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• chore(blockifier): create unit tests for contract_class_manager #2768 👈 (View in Graphite)
• chore(blockifier): add fn add_casm to test_utils::contracts #3138
• main-v0.13.4

This stack of pull requests is managed by Graphite. Learn more about stacking.

[github-actions]

Artifacts upload workflows:

• Started at 2024-12-18T15:30:33Z
• Started at 2024-12-18T15:36:44Z
• Started at 2024-12-18T15:53:55Z
• Started at 2024-12-18T16:11:33Z
• Started at 2024-12-18T16:53:04Z
• Started at 2024-12-19T09:07:48Z
• Started at 2024-12-19T09:08:52Z
• Started at 2024-12-19T09:09:42Z
• Started at 2024-12-19T09:25:14Z
• Started at 2024-12-24T15:12:43Z
• Started at 2024-12-26T10:10:41Z
• Started at 2024-12-26T10:17:01Z
• Started at 2024-12-26T10:18:18Z
• Started at 2024-12-26T10:27:54Z
• Started at 2024-12-26T15:44:28Z
• Started at 2024-12-29T12:35:46Z
• Started at 2025-01-01T08:35:30Z
• Started at 2025-01-01T15:45:43Z
• Started at 2025-01-01T15:47:38Z
• Started at 2025-01-02T10:21:40Z
• Started at 2025-01-02T15:52:45Z
• Started at 2025-01-02T15:53:10Z
• Started at 2025-01-05T15:32:26Z
• Started at 2025-01-05T15:43:38Z
• Started at 2025-01-05T16:46:08Z
• Started at 2025-01-05T16:50:54Z
• Started at 2025-01-06T14:51:09Z
• Started at 2025-01-06T14:53:32Z
• Started at 2025-01-06T14:59:55Z

[noaov1]

Reviewed 7 of 7 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @avi-starkware and @avivg-starkware)

---

crates/blockifier/src/test_utils/contracts.rs line 183 at r3 (raw file):

    }

    pub fn get_casm(&self) -> CompiledClassV1 {

Consider sharing code withget_class.

Code quote:

pub fn get_casm(&self) -> CompiledClassV1 {

---

crates/blockifier/src/state/contract_class_manager_test.rs line 19 at r3 (raw file):

    let mut contract_class = test_contract.get_contract_class();
    // Truncate the sierra program to trigger an error.
    contract_class.sierra_program = contract_class.sierra_program[..100].to_vec();

Nice :)
Is there a way to cause a compilation error by "damaging" the SierraContractClass object?

Code quote:

    // Truncate the sierra program to trigger an error.
    contract_class.sierra_program = contract_class.sierra_program[..100].to_vec();

---

crates/blockifier/src/state/contract_class_manager_test.rs line 77 at r3 (raw file):

        "Sender should be able to send a request successfully"
    );
}

Consider unifying these tests with test_cases.
You can also check: no compiler when cairo-native is off.

Code quote:

#[test]
fn test_sender_with_native_compilation_disabled() {
    let config = ContractClassManagerConfig { run_cairo_native: false, ..Default::default() };
    let manager = ContractClassManager::start(config);
    assert!(manager.sender.is_none(), "Sender should be None when native compilation is disabled");
}

#[test]
fn test_sender_with_native_compilation_enabled() {
    let config = ContractClassManagerConfig { run_cairo_native: true, ..Default::default() };
    let manager = ContractClassManager::start(config);
    assert!(manager.sender.is_some());

    assert!(
        manager.sender.as_ref().unwrap().try_send(create_test_request()).is_ok(),
        "Sender should be able to send a request successfully"
    );
}

---

crates/blockifier/src/state/contract_class_manager_test.rs line 80 at r3 (raw file):

#[test]
fn test_send_request_channel_disconnected() {

Can you please explain this test?

Code quote:

fn test_send_request_channel_disconnected() 

---

crates/blockifier/src/state/contract_class_manager_test.rs line 108 at r3 (raw file):

    let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
        manager.send_compilation_request(second_request);
    }));

Why is the catch_unwind needed?

Code quote:

    let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
        manager.send_compilation_request(second_request);
    }));

---

crates/blockifier/src/state/contract_class_manager_test.rs line 114 at r3 (raw file):

#[test]
fn test_run_compilation_worker_success() {

?

Suggestion:

fn test_process_compilation_request_success() {

---

crates/blockifier/src/state/contract_class_manager.rs line 33 at r3 (raw file):

pub const DEFAULT_COMPILATION_REQUEST_CHANNEL_SIZE: usize = 1000;

#[cfg(all(test, feature = "cairo_native"))]

We will want to test it even when the cairo-native flag is off.

Code quote:

#[cfg(all(test, feature = "cairo_native"))]

[avivg-starkware]

crates/blockifier/src/state/contract_class_manager_test.rs line 19 at r3 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Nice :)
Is there a way to cause a compilation error by "damaging" the SierraContractClass object?

Done

[avivg-starkware]

crates/blockifier/src/state/contract_class_manager_test.rs line 77 at r3 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Consider unifying these tests with test_cases.
You can also check: no compiler when cairo-native is off.

Done.

[avivg-starkware]

crates/blockifier/src/state/contract_class_manager_test.rs line 80 at r3 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Can you please explain this test?

my intention was to check that panic occurs when trying to send massage with disconnected channel, how does it look now?

[avivg-starkware]

crates/blockifier/src/state/contract_class_manager_test.rs line 108 at r3 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why is the catch_unwind needed?

WDYT? not sure if this test is needed and if so, what best way to handle a check that panic doesn't occur

[avivg-starkware]

crates/blockifier/src/state/contract_class_manager_test.rs line 114 at r3 (raw file):

Previously, noaov1 (Noa Oved) wrote…

?

I've changed it to test send_compilation_request and added a separate test for process_compilation_request.
Do you think we need a test for the case wait_on_native = false? using time funcs to ensure enough time elapses for the other thread to finish compiling

[meship-starkware]

Reviewed 2 of 7 files at r2, 1 of 1 files at r3, 1 of 2 files at r5, all commit messages.
Reviewable status: 6 of 7 files reviewed, 8 unresolved discussions (waiting on @avi-starkware, @avivg-starkware, and @noaov1)

---

crates/blockifier/src/state/global_cache.rs line 33 at r3 (raw file):

#[cfg(feature = "cairo_native")]
#[derive(Debug, Clone, PartialEq)]

Can you derive the PartialEq only for the test config?

Code quote:

#[derive(Debug, Clone, PartialEq)]

[avivg-starkware]

Reviewable status: 5 of 7 files reviewed, 7 unresolved discussions (waiting on @avi-starkware, @meship-starkware, and @noaov1)

---

crates/blockifier/src/state/global_cache.rs line 33 at r3 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Can you derive the PartialEq only for the test config?

Done

[avivg-starkware]

crates/blockifier/src/test_utils/contracts.rs line 183 at r3 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Consider sharing code withget_class.

handled code sharing in #3138