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

Proof of Concept: custom scope for check! #39

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

felixwrt
Copy link

I use assert2::check! a lot in my tests, but was missing some flexibility. Especially when using check! in a loop, there's currently no way to prevent panicking after the first iteration in which the check fails. I read #23 and had a slightly different idea that I successfully tried out:

This PR contains the proof of concept of that idea. I've introduced a CheckContext type that tracks whether a check failed and panics when it goes out of scope. This context can be used in the new macro check_ctx!. I've also written two tests that show how these two items can be used. This is the output generated by these tests:

running 3 tests
test __assert2_impl::print::diff::test_div_ceil ... ok
test test::test_check_in_loop ... FAILED
test test::test_check_nested ... FAILED

failures:

---- test::test_check_in_loop stdout ----
Assertion failed at src/lib.rs:385:4:
  assert!( i % 2 == 0 )
with expansion:
  1 == 0
with message:
  Check failed for i=1

Assertion failed at src/lib.rs:385:4:
  assert!( i % 2 == 0 )
with expansion:
  1 == 0
with message:
  Check failed for i=3

Assertion failed at src/lib.rs:385:4:
  assert!( i % 2 == 0 )
with expansion:
  1 == 0
with message:
  Check failed for i=5

thread 'test::test_check_in_loop' panicked at src/lib.rs:372:13:
check failed
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- test::test_check_nested stdout ----
Assertion failed at src/lib.rs:401:3:
  assert!( i > 200 )
with expansion:
  123 > 200

Assertion failed at src/lib.rs:405:3:
  assert!( i % 2 == 0 )
with expansion:
  1 == 0

thread 'test::test_check_nested' panicked at src/lib.rs:372:13:
check failed


failures:
    test::test_check_in_loop
    test::test_check_nested

test result: FAILED. 1 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

error: test failed, to rerun pass `--lib`

I quite like the approach and I think that it's even possible to add support for passing a context to the existing check! macro.

What do you think? Could you see something like this in assert2?

@felixwrt felixwrt marked this pull request as draft January 11, 2025 14:38
@de-vri-es
Copy link
Owner

Hey!

Thanks for the PR. I had been thinking about a mechanism like this as a solution too. The main reason I didn't implement it is because I was still holding out for something like test::mark_failed(). There is some progress towards test::mark_failed(), but it's also nowhere near ready.

In a way, this PR would make it more difficult to switch because we're giving more control over when the panic happens. So changing the macro to not panic at all would be more of a breaking change than it is now (the macro documentation explicitly states that the function might stop panicking at all in the future).

So I'm a bit torn: on the one hand I want to have a solution, and I think the one in this PR is currently the only viable one. On the other hand, I don't really want check!() to panic at all in the future.

@felixwrt
Copy link
Author

Sry that it's been taking me so long to respond...

Couldn't check_ctx have the same note ascheck saying that it might not panic anymore in the future? If failing a test without panicking becomes possible in the future, check_ctx could be changed to not panic and could be deprecated. Existing code would continue to work and could gradually be fixed to resolve the usage of check_ctx.

Am I missing something?

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