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

Feat: Translate Dafny tests to Rust tests #5676

Merged
merged 18 commits into from
Sep 12, 2024
Merged

Conversation

MikaelMayer
Copy link
Member

@MikaelMayer MikaelMayer commented Aug 8, 2024

This PR detects all tests and creates adequate equivalent Rust tests in the same file, by calling methods via top-level functions and translating {:test} attributes to the equivalent #[test] in Rust. Moreover, it translates the {:test} annotation on modules to the equivalent #[cfg(test)] in Rust, so that it makes it possible to exclude entire modules when not in test mode.

I wrote a test that verifies that tests can be executed and that the resulting source code emits #[cfg(test)]

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

This PR detects all tests and creates adequate equivalent Rust tests in the same file, by calling methods via top-level functions and translating `{:test}` attributes to the equivalent `#[test]` in Rust.
Moreover, it translates the `{:test}` annotation on modules to the equivalent `#[cfg(test)]` in Rust, so that it makes it possible to exclude entire modules when not in test mode.

I wrote a test that verifies that tests can be executed and that the resulting source code emits `#[cfg(test)]`
MikaelMayer added a commit that referenced this pull request Aug 8, 2024
Copy link
Member

@robin-aws robin-aws left a comment

Choose a reason for hiding this comment

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

Good feature to have, just some suggestions on packaging.

I do still feel it should be possible for the --include-test-runner approach to work for Rust eventually too, we just need to go deeper into the Rust kernel to use what ever panic recovery cargo test itself uses, so I'm trying not to close that door now.

Copy link
Member

Choose a reason for hiding this comment

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

Exciting! :)

Comment on lines 1870 to 1872
if HasTestAttribute(mod.attributes) {
attributes := [R.RawAttribute("#[cfg(test)]")];
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather not do exactly this because it's not a complete solution. {:test} on modules doesn't have any meaning at the Dafny level as it does for methods. And it's likely to be misinterpreted as "This scope contains test methods" as it is in some testing frameworks, rather than "This code should only be compiled in test mode". I suspect users won't realize they have to also put {:test} on helper modules as well.

If you really want this I'd suggest {:rust_test_cfg} or something like that instead.

Copy link
Member Author

@MikaelMayer MikaelMayer Sep 12, 2024

Choose a reason for hiding this comment

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

I'll go with {:rust_cfg_test} then, it will be easier later to have Backend-specific typed options.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also updated the docs to mention the two attributes in the Rust-specific section.

@@ -2084,6 +2093,27 @@ module {:extern "DCOMP"} DafnyToRustCompiler {
);
s := s + [R.ImplDecl(i)];
}
// Add test methods
Copy link
Member

Choose a reason for hiding this comment

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

We should only do this if the RunAllTestsMainMethod.IncludeTestRunner option is off - we already do that for C#: https://github.com/dafny-lang/dafny/blob/master/Source/DafnyCore/Backends/CSharp/CsharpCodeGenerator.cs#L3454

The two ways of implementing {:test} conflict subtley.

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand that it might be necessary for C# for some reason, but I don't see a reason to do that in Rust.
Currently "dafny test" is the only command that forces to turn on this option because that way it won't create test fixtures, just run them regularly.
However, in the future, "dafny test" for Rust should not include the Test Runner but call "cargo test" instead, so it should safely ignore this IncludeTestRunner.
That is, unless you have another explanation about why it is needed?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I'd want to make dafny test call cargo test instead, since the output will be different from the other backends.

But on second look I don't think it's harmful to add #[test] even if you're running the common test runner, so I won't object to leaving it in for simplicity.

Copy link
Member

Choose a reason for hiding this comment

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

LGTM, but just noticed the PR number in the file name is wrong.

robin-aws
robin-aws previously approved these changes Sep 12, 2024
@MikaelMayer MikaelMayer enabled auto-merge (squash) September 12, 2024 20:52
@MikaelMayer MikaelMayer merged commit e958f79 into master Sep 12, 2024
21 checks passed
@MikaelMayer MikaelMayer deleted the feat-5666-cargo-test branch September 12, 2024 22:28
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