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

Fix handling of test_case with custom name #30

Closed
wants to merge 9 commits into from

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Oct 5, 2023

See individual commits.

Still missing is guidance on how to write #[test_case(...)] tests with multiple cases. For example

#[test_log::test(test_case::test_case(-2, -4; "one"))]
#[test_log::test(test_case::test_case(-4, -2; "two"))]
fn it_works(x: i8, y: i8) {
  // ...
}

doesn't work.

I think the approach of wrapping other test macros such as tokio::test and test_case::test_case in the
test_log::test macro ends up being inflexible for such cases and it's probably better to abandon it in favor of
parsing other attributes on the test function and inserting #[test] only if nothing else has already done it. This
would mean that ordering of attributes would start to matter but this is already the case when test_case::test_case is
composed with tokio::test; test_case has to come first or else compilation fails with "functions used as tests can
not have any arguments".

@tamird
Copy link
Contributor Author

tamird commented Oct 8, 2023

@d-e-s-o would you mind having a look at this?

@d-e-s-o
Copy link
Owner

d-e-s-o commented Oct 9, 2023

@d-e-s-o would you mind having a look at this?

Sorry for the delay. Seems like a good set of simplifications. Can you please remove the various formatting changes, though?

Regarding the second to last commit, can we remove tracing from the dependency list in the README now?

@d-e-s-o
Copy link
Owner

d-e-s-o commented Oct 9, 2023

I think the approach of wrapping other test macros such as tokio::test and test_case::test_case in the
test_log::test macro ends up being inflexible for such cases and it's probably better to abandon it in favor of
parsing other attributes on the test function and inserting #[test] only if nothing else has already done it. This
would mean that ordering of attributes would start to matter but this is already the case when test_case::test_case is
composed with tokio::test; test_case has to come first or else compilation fails with "functions used as tests can
not have any arguments".

Hm, but that isn't what is being done, or am I misreading something?

@d-e-s-o
Copy link
Owner

d-e-s-o commented Oct 9, 2023

Can you please remove the various formatting changes, though?

Or for the actual code reuse one of my other rustfmt files. But please keep toml.

Add a job to check formatting in Github Actions.

Copied from https://github.com/d-e-s-o/apca/blob/main/rustfmt.toml.
There's no need to inspect the test case so deeply; all this macro wants
to do is insert a prelude - so do that and expand the rest as-is.

This removes the inner function `test_impl`, which is the suspected
cause of d-e-s-o#28.

Closes d-e-s-o#28.
This function doesn't serve much of a purpose now. In the next commit,
we need to change how we handle its inputs; better to just inline it to
reduce churn.
Parsing `test_case::test_case(-2, -4; "my test name")` as
`AttributeArgs` would fail because a semicolon is illegal in
`AttributeArgs`. There's nothing of value being done after this parsing,
so better to replace it with a simple fallback if the wrapper attribute
is absent.
Add minimal taplo config that retains current formatting.
This is otherwise not referenced. Remove the need to add a dependency on
`tracing` from the README.
@tamird
Copy link
Contributor Author

tamird commented Oct 9, 2023

Can you please remove the various formatting changes, though?

Or for the actual code reuse one of my other rustfmt files. But please keep toml.

Done.

Regarding the second to last commit, can we remove tracing from the dependency list in the README now?

Done.

Hm, but that isn't what is being done, or am I misreading something?

Correct, that is not what's being done. That's a separate commentary. Changing that would break existing users, so I didn't do it here.

@d-e-s-o
Copy link
Owner

d-e-s-o commented Oct 10, 2023

Sorry, running behind on this request. Will try to merge this by the end of the week.

@d-e-s-o
Copy link
Owner

d-e-s-o commented Oct 11, 2023

I added CHANGELOG entries and merged the changes with a few minor modifications. Thanks for taking the time to contribute to the project!

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