test: Remove `mock` crate #711

igamigo · 2024-05-28T19:40:48Z

Addresses #700
This PR removes the mock crate, and moves its code into one of the other crates. Some details worth mentioning:

In all cases, the code is behind the testing flag and behind a new testing module on their respective locations. I was thinking the name of the module could be mock but it feels like it's easier to relate the modules to their features directly by name, but I can change it to mock if we want to keep that nomenclature. The alternative is also that we introduce a new feature mock, in case we want to decouple the previously-existing testing features from the new code, but I think it's unnecessary at this point.
- The hierarchy of modules is such that each module contains a testing submodule where applicable. For instance, mock's NoteBuilder now exists on miden_objects::notes::testing::NoteBuilder. This was based off of the fact that some of the structs already contained mock code behind the feature flag.
Most of the code is moved to objects since it's directly related to the domain structs. Some of it is moved to miden-tx, like code related to MockHost and MockDataStore, and some other to miden-lib (depending mainly on the dependencies of the moved code). Because miden-lib has many kernel tests which end up using these structs, a new dev dependency for miden-tx was introduced.
As a consequence, some duplicated code was removed and very minor changes were introduced to the code (removing mock_block_header in favor of BlockHeader::mock(), for example).
The mock binary was removed as well as it's not being used anywhere.

The idea is for the next PR to introduce some refactors to the tests and refactor code .

…nto igamigo-tests-fixes

mFragaBA

Nice work, testing helpers/mocks have been needing some love for a while now. I left some comments. Most are questions regarding why moving something from mock to a specific subcrate instead of another one.

miden-tx/src/host/testing/procedures.rs

miden-tx/src/host/testing/utils.rs

miden-tx/src/host/tx_authenticator.rs

miden-tx/src/lib.rs

mFragaBA · 2024-05-30T20:12:07Z

miden-tx/src/lib.rs

@@ -21,6 +21,8 @@ mod compiler;
 pub use compiler::{ScriptTarget, TransactionCompiler};

 mod executor;
+#[cfg(feature = "testing")]
+pub use executor::testing::MockDataStore;


should we export a testing module like in other crates?

This was replaced by the flatter structure

bench-tx/src/main.rs

mFragaBA · 2024-05-30T20:40:39Z

miden-lib/src/testing/mod.rs

-
-// HELPER FUNCTIONS
-// ================================================================================================
-
-fn build_dummy_tx_program() -> Program {
-    let operations = vec![Operation::Push(ZERO), Operation::Drop];
-    let span = miden_objects::vm::CodeBlock::new_span(operations);
-    Program::new(span)
-}


I noticed this was moved to miden_objects, but considering this is a tx_program, wouldn't it also make sense to keep in miden_tx?

Agreed, I had it in miden-lib at first but Operation was not available there, so I moved it down because miden-lib did not have miden-tx as a dependenciy. But since I added it as a dev dependency now, I think I can move it there.
EDIT: Nvm, can't do this because this is not test code, so the dev dependency does not apply.

mFragaBA · 2024-05-30T20:40:51Z

miden-lib/src/testing/mod.rs

Why was this moved into miden-lib instead of miden-tx (seemed weird considering that it was previously under mock/transactions.rs)

miden-lib uses code from miden-tx to test code there (like MockDataStore, etc.). Because of this, the functions could not be originally used on miden-lib. However I later introduced it as a dev dependency so they can be moved now, going to test soon.

miden-tx/Cargo.toml

miden-tx/src/host/testing/procedures.rs

bobbinth

Looks good! Thank you! I left a few small comments inline. A couple of general comments:

In all cases, the code is behind the testing flag and behind a new testing module on their respective locations. I was thinking the name of the module could be mock but it feels like it's easier to relate the modules to their features directly by name

Yes, this makes sense.

The hierarchy of modules is such that each module contains a testing submodule where applicable. For instance, mock's NoteBuilder now exists on miden_objects::notes::testing::NoteBuilder. This was based off of the fact that some of the structs already contained mock code behind the feature flag.

There is another way to do it: we could just have a single testing module per crate and have sub-modules under it for specific use cases. This would be a bit cleaner as all testing code would be isolated in one place rather than being throughout the crate. But I also haven't looked deeply enough into this to see if it causes any issues.

Because miden-lib has many kernel tests which end up using these structs, a new dev dependency for miden-tx was introduced.

This causes a circular dependency - but I'm guessing this is not a problem? An alternative would be to move all tests which require miden-tx to miden-tx crate.

.pre-commit-config.yaml

objects/src/testing.rs

objects/src/assets/testing.rs

objects/src/accounts/testing/chain.rs

igamigo · 2024-05-31T18:32:09Z

This causes a circular dependency - but I'm guessing this is not a problem? An alternative would be to move all tests which require miden-tx to miden-tx crate.

Correct, it does not right now. I thought of moving the tests although then the code that is meant to test mainly the kernel would be located somewhere else.

There is another way to do it: we could just have a single testing module per crate and have sub-modules under it for specific use cases. This would be a bit cleaner as all testing code would be isolated in one place rather than being throughout the crate. But I also haven't looked deeply enough into this to see if it causes any issues.

Yeah, I originally did it like this because some code was already there with that hierarchy, but I think this makes more sense.

bobbinth · 2024-05-31T18:42:13Z

There is another way to do it: we could just have a single testing module per crate and have sub-modules under it for specific use cases. This would be a bit cleaner as all testing code would be isolated in one place rather than being throughout the crate. But I also haven't looked deeply enough into this to see if it causes any issues.

Yeah, I originally did it like this because some code was already there with that hierarchy, but I think this makes more sense.

Let's go with the more "self-contained" approach then (i.e., a single testing module per crate).

…nto igamigo-tests-fixes

igamigo · 2024-05-31T21:41:51Z

This causes a circular dependency - but I'm guessing this is not a problem? An alternative would be to move all tests which require miden-tx to miden-tx crate.

I ended up moving all tests that required miden-tx to that crate (essentially all of them)

bobbinth

Looks good! Thank you! I left a few small comments inline.

miden-lib/src/tests/mod.rs

miden-tx/src/lib.rs

miden-tx/tests/integration/main.rs

miden-tx/src/kernel_tests/mod.rs

miden-tx/src/testing/mod.rs

…nto igamigo-tests-fixes

bobbinth

Looks good! Thank you! I left one small comment inline.

miden-tx/src/lib.rs

* test: Remove mock crate * Test file * Fix added test * Move code * Fix tests * Fix build * Set vm-processor feature * Changelog and docs * Changelog and docs * Address reviews * Move some more code around * Move some more code around * Move kernel tests to miden-tx * Remove file * Lints * Move MockHost (this is unused) * Correct kernel path * Address reviews * Remove unnecessary assignment * change kernel tests guard

igamigo changed the title ~~test: Remove duplicate code~~ test: Remove mock crate May 29, 2024

test: Remove mock crate

3730e4b

igamigo force-pushed the igamigo-tests-fixes branch from d3b6d59 to 3730e4b Compare May 29, 2024 22:52

igamigo added 5 commits May 29, 2024 20:14

Test file

160d761

Merge branch 'next' of https://github.com/0xPolygonMiden/miden-base i…

edce4f4

…nto igamigo-tests-fixes

Fix added test

011b0ea

Move code

ace36b2

Fix tests

cfadf25

igamigo marked this pull request as ready for review May 30, 2024 15:46

igamigo added 5 commits May 30, 2024 14:43

Merge branch 'next' of https://github.com/0xPolygonMiden/miden-base i…

1b0dea0

…nto igamigo-tests-fixes

Fix build

4d2ed84

Set vm-processor feature

f537703

Changelog and docs

00dcff9

Changelog and docs

c84a54e

mFragaBA reviewed May 30, 2024

View reviewed changes

Address reviews

725d5a2

bobbinth reviewed May 30, 2024

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

objects/src/testing.rs Outdated Show resolved Hide resolved

objects/src/assets/testing.rs Outdated Show resolved Hide resolved

objects/src/accounts/testing/chain.rs Outdated Show resolved Hide resolved

igamigo added 6 commits May 31, 2024 16:11

Move some more code around

744c6fd

Move some more code around

4a5c8ca

Move kernel tests to miden-tx

a559489

Remove file

09a7857

Merge branch 'next' of https://github.com/0xPolygonMiden/miden-base i…

5608169

…nto igamigo-tests-fixes

Lints

4783232

igamigo added 2 commits May 31, 2024 18:48

Move MockHost (this is unused)

a87098d

Correct kernel path

9b03d1f

bobbinth approved these changes May 31, 2024

View reviewed changes

Address reviews

0471697

igamigo added 2 commits June 3, 2024 11:27

Remove unnecessary assignment

5a298b9

Merge branch 'next' of https://github.com/0xPolygonMiden/miden-base i…

95f4fba

…nto igamigo-tests-fixes

bobbinth approved these changes Jun 4, 2024

View reviewed changes

miden-tx/src/lib.rs Outdated Show resolved Hide resolved

change kernel tests guard

8246197

igamigo merged commit 68ff4ec into next Jun 5, 2024
9 checks passed

igamigo deleted the igamigo-tests-fixes branch June 5, 2024 00:49

bobbinth mentioned this pull request Jul 1, 2024

Mock updates #487

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Remove `mock` crate #711

test: Remove `mock` crate #711

igamigo commented May 28, 2024 •

edited

Loading

mFragaBA left a comment

mFragaBA May 30, 2024

igamigo May 31, 2024

mFragaBA May 30, 2024

igamigo May 30, 2024 •

edited

Loading

mFragaBA May 30, 2024

igamigo May 31, 2024 •

edited

Loading

bobbinth left a comment

igamigo commented May 31, 2024

bobbinth commented May 31, 2024

igamigo commented May 31, 2024 •

edited

Loading

bobbinth left a comment

bobbinth left a comment

test: Remove mock crate #711

test: Remove mock crate #711

Conversation

igamigo commented May 28, 2024 • edited Loading

mFragaBA left a comment

Choose a reason for hiding this comment

mFragaBA May 30, 2024

Choose a reason for hiding this comment

igamigo May 31, 2024

Choose a reason for hiding this comment

mFragaBA May 30, 2024

Choose a reason for hiding this comment

igamigo May 30, 2024 • edited Loading

Choose a reason for hiding this comment

mFragaBA May 30, 2024

Choose a reason for hiding this comment

igamigo May 31, 2024 • edited Loading

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

igamigo commented May 31, 2024

bobbinth commented May 31, 2024

igamigo commented May 31, 2024 • edited Loading

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

test: Remove `mock` crate #711

test: Remove `mock` crate #711

igamigo commented May 28, 2024 •

edited

Loading

igamigo May 30, 2024 •

edited

Loading

igamigo May 31, 2024 •

edited

Loading

igamigo commented May 31, 2024 •

edited

Loading