Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests #112

Merged
merged 20 commits into from
Feb 3, 2024
Merged

Tests #112

merged 20 commits into from
Feb 3, 2024

Conversation

lukacan
Copy link
Collaborator

@lukacan lukacan commented Jan 30, 2024

This PR should improve/introduce

  • tests for functions generating fuzz source codes for:
    • accounts_snapshots.rs
    • fuzz_instructions.rs
  • tests macros introduced with the fuzzer:
    • DisplayIx
    • FuzzDeserialize
    • FuzzTestExecutor
    • fuzz_trd

Info

  • macros are tested with macrotest crate
    • this crate generates expected code after macros are expanded
    • within the subsequent test runs expanded macros are compared to the generated code
  • previous tests that were already implemented inside the client crate were moved to the tests folder within the client folder
  • fuzzer_generator.rs code was updated so that the order of accounts inside FuzzAccounts is always the same so we can compare the generated source code to the expected one.
  • github workflow was updated to contain installation for cargo expand

@lukacan lukacan requested a review from Ikrk January 30, 2024 08:55
Copy link
Contributor

@Ikrk Ikrk left a comment

Choose a reason for hiding this comment

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

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.

@lukacan
Copy link
Collaborator Author

lukacan commented Jan 31, 2024

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
Copy link
Contributor

Ikrk commented Feb 3, 2024

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: eupn/macrotest#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.

Copy link
Contributor

@Ikrk Ikrk left a comment

Choose a reason for hiding this comment

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

Otherwise it looks good now.

@Ikrk Ikrk merged commit 9776295 into develop Feb 3, 2024
7 checks passed
@Ikrk Ikrk deleted the tests branch February 3, 2024 00:30
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.

2 participants