Feature: make test_utils macros public #1599

odesenfans · 2024-02-05T13:37:04Z

This PR makes all macros in the test_utils module public to allow for easier testing of hints (and other features) living in other crates/repositories.

Fixes #1595.

Checklist

Linked to Github Issue
Unit tests added
Integration tests added.
This change requires new documentation.
- Documentation has been added/updated.
- CHANGELOG has been updated.

fmoletta · 2024-02-05T13:41:22Z

vm/src/utils.rs

@@ -76,7 +76,7 @@ pub mod test_utils {
            Into::<num_bigint::BigInt>::into($val)
        };
    }
-    pub(crate) use bigint;
+    pub use bigint;


This is not really vm-related, so I don't think it should be included in the vm's public api. Moreover, we may choose to remove the usage of BigInt from the VM altogether and remove this macro too.

Makes sense. I made it public out of simplicity because keeping it pub(crate) in a now public module induces a few unused import warnings. I'll find a solution.

fmoletta · 2024-02-05T13:41:30Z

vm/src/utils.rs

@@ -87,15 +87,15 @@ pub mod test_utils {
            num_bigint::BigInt::parse_bytes($val.as_bytes(), $opt).expect("Couldn't parse bytes")
        };
    }
-    pub(crate) use bigint_str;
+    pub use bigint_str;


This is not really vm-related, so I don't think it should be included in the vm's public api. Moreover, we may choose to remove the usage of BigInt from the VM altogether and remove this macro too.

fmoletta · 2024-02-05T13:41:55Z

vm/src/utils.rs


    #[macro_export]
    macro_rules! biguint {
        ($val : expr) => {
            Into::<num_bigint::BigUint>::into($val as u128)
        };
    }
-    pub(crate) use biguint;
+    pub use biguint;


This is not really vm-related, so I don't think it should be included in the vm's public api. Moreover, we may choose to remove the usage of BigUint from the VM altogether and remove this macro too.

fmoletta · 2024-02-05T13:42:04Z

vm/src/utils.rs

@@ -106,7 +106,7 @@ pub mod test_utils {
            num_bigint::BigUint::parse_bytes($val.as_bytes(), $opt).expect("Couldn't parse bytes")
        };
    }
-    pub(crate) use biguint_str;
+    pub use biguint_str;


This is not really vm-related, so I don't think it should be included in the vm's public api. Moreover, we may choose to remove the usage of BigUint from the VM altogether and remove this macro too.

fmoletta · 2024-02-05T13:43:14Z

vm/src/utils.rs

@@ -155,8 +158,9 @@ pub mod test_utils {
            }
        };
    }
-    pub(crate) use memory_from_memory;
+    pub use memory_from_memory;


This is an auxiliary macro used to implement the memory macro, it shouldn't be made public

These macros need to be public to make the memory and segments macros usable. Users need to import them as well.

fmoletta · 2024-02-05T13:43:27Z

vm/src/utils.rs

@@ -189,17 +193,19 @@ pub mod test_utils {
            }
        };
    }
-    pub(crate) use memory_inner;
+    pub use memory_inner;


This is an auxiliary macro used to implement the memory macro, it shouldn't be made public

fmoletta · 2024-02-05T13:44:52Z

vm/src/utils.rs


-    pub(crate) use crate::stdlib::{collections::BTreeMap, sync::Arc};
+    pub use crate::stdlib::{collections::BTreeMap, sync::Arc};


This is internal to the VM, we shouldn't make it public

Only solution I found for this is to split the import in two and

#[allow(unused_imports)] pub(crate) use crate::stdlib::collections::BTreeMap; pub(crate) use crate::stdlib::sync::Arc;

Tell me if you have a better solution.

fmoletta · 2024-02-05T13:46:13Z

vm/src/utils.rs

    pub(crate) use crate::types::program::HintsCollection;
-    pub(crate) use crate::types::program::Program;
+    pub use crate::types::program::Program;


Program is already public, we should use the full path in the macro rather than re-exporting it here

Fixes lambdaclass#1595.

odesenfans · 2024-02-21T23:27:13Z

This PR should be mergeable. Most of the macros need to be public as they are used in public macros. I addressed all the comments that could be.

juanbono · 2024-03-01T14:58:42Z

vm/src/vm/vm_memory/memory.rs

@@ -92,8 +92,8 @@ impl AddressSet {
 }

 pub struct Memory {
-    pub(crate) data: Vec<Vec<Option<MemoryCell>>>,


Why did you made these 2 fields public? they are private to make it easier to refactor the representation and improve it. There are many methods for retrieving and inspecting the Memory and pretty-printing it (Display instance).

Same, some macros access these fields directly so they must be public to use the macros out of tree. I'll try to think of another solution.

juanbono · 2024-03-01T14:59:16Z

vm/src/vm/vm_memory/memory_segments.rs

@@ -15,7 +15,7 @@ use crate::{
 pub struct MemorySegmentManager {
    pub segment_sizes: HashMap<usize, usize>,
    pub segment_used_sizes: Option<Vec<usize>>,
-    pub(crate) memory: Memory,
+    pub memory: Memory,


this should not be made public.

Some of the macros in test_utils require access to this field. As the goal is to make the macros usable out of tree, the easy solution is to make this field public. I understand the concern though. Any suggestion on how to work around the issue?

Any suggestion on how to work around the issue?

It depends on what you want to do in the tests. Maybe you could just use the insert family of functions.

juanbono

See comments

fmoletta · 2024-04-08T15:22:54Z

Hello! Making these macros public in the current state involves making a lot of fields internal to the VM public which is not desirable at all. I think we should address this task in a different way. What these macros currently do can be implemented using the vm's public api, so we should re-implement or implement new macros that use the public api instead.

odesenfans · 2024-04-10T14:54:42Z

Ok @fmoletta then let's close this PR, no worries.

odesenfans requested review from igaray, Oppen, fmoletta, entropidelic, juanbono and pefontana as code owners February 5, 2024 13:37

odesenfans force-pushed the msl/make-test-macros-public branch from 1e0b98c to bebb3ed Compare February 5, 2024 13:37

fmoletta reviewed Feb 5, 2024

View reviewed changes

odesenfans added 4 commits February 22, 2024 00:26

Feature: make test_utils macros public

556bf31

Fixes lambdaclass#1595.

add pub(crate) test-only module for bigint macros

1c0e688

make segments! usable

5be192f

appease God Clippy

c13160a

odesenfans force-pushed the msl/make-test-macros-public branch from 9f208d2 to c13160a Compare February 21, 2024 23:26

juanbono reviewed Mar 1, 2024

View reviewed changes

juanbono requested changes Mar 1, 2024

View reviewed changes

odesenfans closed this Apr 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: make test_utils macros public #1599

Feature: make test_utils macros public #1599

odesenfans commented Feb 5, 2024 •

edited

fmoletta Feb 5, 2024

odesenfans Feb 5, 2024

fmoletta Feb 5, 2024

fmoletta Feb 5, 2024

fmoletta Feb 5, 2024

fmoletta Feb 5, 2024

odesenfans Feb 5, 2024

fmoletta Feb 5, 2024

fmoletta Feb 5, 2024

odesenfans Feb 5, 2024

fmoletta Feb 5, 2024

odesenfans commented Feb 21, 2024

juanbono Mar 1, 2024

odesenfans Mar 1, 2024

juanbono Mar 1, 2024

odesenfans Mar 1, 2024

Oppen Mar 8, 2024

juanbono left a comment

fmoletta commented Apr 8, 2024

odesenfans commented Apr 10, 2024


		pub(crate) use crate::stdlib::{collections::BTreeMap, sync::Arc};
		pub use crate::stdlib::{collections::BTreeMap, sync::Arc};

Feature: make test_utils macros public #1599

Feature: make test_utils macros public #1599

Conversation

odesenfans commented Feb 5, 2024 • edited

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odesenfans commented Feb 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanbono left a comment

Choose a reason for hiding this comment

fmoletta commented Apr 8, 2024

odesenfans commented Apr 10, 2024

odesenfans commented Feb 5, 2024 •

edited