-
Notifications
You must be signed in to change notification settings - Fork 15
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
base: master
Are you sure you want to change the base?
Conversation
17f2bcb
to
7ff1cf1
Compare
@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 https://github.com/s-arash/ascent/blob/master/.github/workflows/rust.yml#L52-L58 |
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 |
7ff1cf1
to
884c612
Compare
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 As such by including They also allow one to —for example— confidently make ambitious refactors in 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
Adding |
884c612
to
a3a860a
Compare
I've updated the tests and gave the |
a3a860a
to
7c760e9
Compare
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 ofTRYEXPAND=overwrite cargo test --manifest-path "ascent_tests/Cargo.toml"
to update the snapshots).