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

Add basic suite of macro snapshot tests #35

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

regexident
Copy link
Contributor

@regexident regexident commented Mar 10, 2024

This PR migrates a couple of the existing macro tests to a more mature (vs. the current scratchpad.rs approach) snapshot testing suite based on the tryexpand crate, which supports checking macros for the expanded output or emitted error messages, as well as (optionally) type-checking or even running the generated code (and checking the output).

One of the benefits of this approach is that one gets neat diffs in the console when running tests, while also allowing for running multiple tests without having them step on each other's feet while writing to scratchpad.rs.

Additionally the snapshot test files allow for providing nice sneak peek of the code that ascent!{ … } expands to.

Note that these tests are currently failing due to the non-deterministic nature of the implementation in ascent_codegen.rs. Once rebased onto #34 these tests should start passing however (they may need one last run of TRYEXPAND=overwrite cargo test --manifest-path "ascent_tests/Cargo.toml" to update the snapshots).

@regexident
Copy link
Contributor Author

@s-arash the snapshot tests should now pass and help in catching regressions (e.g. with respect to build-determinism) in the future.


It isn't exactly clear to me what your workflow is with scratchpad.rs so I may be wrong, but I think these snapshot tests should be able to supersede it, thus eliminating edge cases like this:

https://github.com/s-arash/ascent/blob/master/.github/workflows/rust.yml#L52-L58

@s-arash
Copy link
Owner

s-arash commented May 3, 2024

Hi @regexident.

I think checking for the exact expansion output is not the right way to test Ascent, since the exact output can change quite frequently. What should be tested is the behavior of generated code.

Your point about scratchpad.rs is valid. I use it mainly as a development/debugging tool, i.e., see what an Ascent program expands to (before recursive macro expansion), and have the code checked by the compiler. I understand the way I currently do it is not ideal, and I should do something about it.

@regexident
Copy link
Contributor Author

I think checking for the exact expansion output is not the right way to test Ascent, since the exact output can change quite frequently. What should be tested is the behavior of generated code.

Totally agree! That said my intention with this PR is not to replace the existing tests with expansion tests (my apologies if my wording above was misleading in that regard), but to add macro expansion tests as one of many different puzzle pieces required for reliably testing proc-macro-based crates.

Also while comparing snapshots of macro expansions is one of the things that tryexpand can do (and arguably the most important one as there otherwise isn't any built-in support for testing macro expansions in the language), it certainly isn't the only one: the crate also offers support for further checking correctness by optionally also running cargo check (via .and_check()), cargo run(via .and_run()) or cargo test (via .and_run_tests()) for each test program and snapshotting the corresponding stderr/stdout output and catching regressions/changes.

As such by including println!(…); or assert!(…); in the test program you can (if you want to) verify multiple test dimensions (expand, build, run) with only a single test program.

They also allow one to —for example— confidently make ambitious refactors in ascent_macros while being 100% certain that the generated code remains unchanged. Something that merely running logic tests against the artifacts on its own can't provide.

Another aspect where these snapshot tests shine: testing expansion diagnostics. How else would you write tests that check whether your code emits the correct warning/error diagnostics in certain situations if all you had were runtime behavioral tests?

As a project owner snapshot tests further more provide a convenient way to quickly check the effective changes of incoming PRs from contributors for possible undesired changes that might not be easily recognizable from the process macro logic itself.

But as I said above the snapshot tests are just one part on the testing tool-belt for testing macro-based crates like ascent.

Whether or not to include logic tests in the test programs themselves is a matter of taste. I would personally try to keep derive and logic tests separate, where possible (and only integrate them if otherwise you'd end up duplicating code over and over between macro and logic tests) and so that's the approach I've taken with my enumcapsulate crate with its derive-tests.rs and logic-tests.rs neatly separated.

Your point about scratchpad.rs is valid. I use it mainly as a development/debugging tool, i.e., see what an Ascent program expands to (before recursive macro expansion), and have the code checked by the compiler. I understand the way I currently do it is not ideal, and I should do something about it.

Adding scratchpad.rs to .gitignore would eliminate the "running cargo test dirties the repo" issue. But would not eliminate the issue of having to make code edits in order to run (or not run) individual expansions on cargo test. Your mention of "before recursive macro expansion" is a very good point however as expansion snapshot tests would of course only test the final code, so in this respect it's not a full replacement. On the flip side one of the advantages of using snapshot tests over say the scratchpad approach is that it already comes with ergonomic diffing support built in.

@regexident
Copy link
Contributor Author

I've updated the tests and gave the tc tests some minimal unit tests to illustrate how one would combine expansion with minimal logic tests

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.

None yet

2 participants