Ackee-Blockchain / trident

Rust-based framework to Fuzz Solana programs, designed to help you ship secure code.
https://ackee.xyz/trident/docs/latest/
MIT License
209 stars 19 forks source link

Tests #112

Closed lukacan closed 9 months ago

lukacan commented 9 months ago

This PR should improve/introduce

Info

lukacan commented 9 months ago

It looks like you have moved the unit tests from commander.rs and config.rs to the integration level. Why? I think it does not make sense and it has the side effect that the functions are now public only for the purpose of tests and also you had to add the two new modules __private and constants. I suggest to revert these changes to the previous state.

reverted. However, I left this code within lib.rs:

pub use trdelnik_derive_displayix::DisplayIx; pub use trdelnik_derive_fuzz_deserialize::FuzzDeserialize; pub use trdelnik_derive_fuzz_test_executor::FuzzTestExecutor;

I`m not sure why, but it is not possible to use: use trdelnik_derive_displayix::DisplayIx; use trdelnik_derive_fuzz_deserialize::FuzzDeserialize; use trdelnik_derive_fuzz_test_executor::FuzzTestExecutor;

inside the test files for the corresponding macros. The same goes for: use trdelnik_client::fuzzing::DisplayIx; ... with --features fuzzing specified with cargo test

Macros do not expand in both cases. However, specifying: pub use trdelnik_derive_displayix::DisplayIx; pub use trdelnik_derive_fuzz_deserialize::FuzzDeserialize; pub use trdelnik_derive_fuzz_test_executor::FuzzTestExecutor; inside lib.rs and then: use trdelnik_client::DisplayIx; ... inside corresponding test files works.

I`m not sure if this is expected behavior or not.

Ikrk commented 9 months ago

It looks like you have moved the unit tests from commander.rs and config.rs to the integration level. Why? I think it does not make sense and it has the side effect that the functions are now public only for the purpose of tests and also you had to add the two new modules __private and constants. I suggest to revert these changes to the previous state.

reverted. However, I left this code within lib.rs:

pub use trdelnik_derive_displayix::DisplayIx; pub use trdelnik_derive_fuzz_deserialize::FuzzDeserialize; pub use trdelnik_derive_fuzz_test_executor::FuzzTestExecutor;

I`m not sure why, but it is not possible to use: use trdelnik_derive_displayix::DisplayIx; use trdelnik_derive_fuzz_deserialize::FuzzDeserialize; use trdelnik_derive_fuzz_test_executor::FuzzTestExecutor;

inside the test files for the corresponding macros. The same goes for: use trdelnik_client::fuzzing::DisplayIx; ... with --features fuzzing specified with cargo test

Macros do not expand in both cases. However, specifying: pub use trdelnik_derive_displayix::DisplayIx; pub use trdelnik_derive_fuzz_deserialize::FuzzDeserialize; pub use trdelnik_derive_fuzz_test_executor::FuzzTestExecutor; inside lib.rs and then: use trdelnik_client::DisplayIx; ... inside corresponding test files works.

I`m not sure if this is expected behavior or not.

I have looked into it and unfortunately there is no elegant solution :-/ Indeed it does not work, because the fuzzing feature is not set in the first place. It should be possible to activate the feature via the macrotest's expands_args function such as:

macrotest::expand_args("tests/test_data/fuzzer_macros/fuzz_display_ix.rs", &["--features", "fuzzing"]);

Unfortunately it does not work due to a bug in macrotest: https://github.com/eupn/macrotest/issues/94 The workaround is to use the latest version from Github by using

macrotest = { git = "https://github.com/eupn/macrotest.git", branch = "master" }

in the Cargo.toml file, but again, there is another bug in this version and the dependencies inherited from the workspace such as trdelnik-test = { workspace = true } are not parsed properly. Here the workaround is not to inherit the dependencies from the workspace and then it works.

However this is probably even worse than exposing the three macros which was the original workaround.