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

Check panics early when run in a loop #23

Open
sargunv opened this issue Oct 11, 2021 · 7 comments
Open

Check panics early when run in a loop #23

sargunv opened this issue Oct 11, 2021 · 7 comments

Comments

@sargunv
Copy link

sargunv commented Oct 11, 2021

Hi, I found this library when looking for a way to run tests with multiple assertions that all evaluate and report errors before panicking at the end. From reading your docs, it appears that the check! macro in this library is intended to do that. However, it doesn't appear to be working for me when run in a loop (I assume it only works within a single scope?). Here's my use case:

I'm writing a program designed to process a builtin config with many different "command" definitions, and for ease of use the tests for each command are embedded in the same config. I'd like my test to run and report on each "test" in the config, so one failing test doesn't stop the rest from being run.

image

I've deliberately tweaked the above config the ensure two different tests fail. However, when running the tests, only one assertion failed is reported before the test panics. See my test code and test output below. You can see the check! is run in a loop.

image

I tried putting two check! calls back to back in the same scope, and that did work as expected; both reported failure before the panic. So, it appears multiple check! calls are limited to running together only if they exist in the same scope.

I assume this is by design and not a bug, but I'd love a feature in this repo that lets me check and report on multiple conditions dynamically within a loop regardless of scope.

@de-vri-es
Copy link
Owner

de-vri-es commented Oct 11, 2021

You are correct that this only works within the same scope. However, I also totally agree with the behaviour you want. I want it too ;)

The biggest problem now is that we must panic to fail a test. The best thing we can do is delay the panic until a later point. So the check!() macro does that with a scope guard that panics when dropped. I can't think of anything better currently :(

Something like ::test::fail() to fail the current test without panicking would help, but that requires support from the standard library. I'm open to other suggestions too, but keep in mind it should work correctly with asynchronous code too.

@sargunv
Copy link
Author

sargunv commented Oct 11, 2021

I'm not really familiar with the capabilities of macros in Rust, but the only real solution I can think of would probably have a macro like check! that only prints the error, doesn't fail the test, but does return the pass/fail state of the test, and then I can panic manually at the end based on those results. That way, I get the multiple error reporting from the check!-like macro but can use fail in whatever way I want.

@m-ou-se
Copy link
Collaborator

m-ou-se commented Oct 11, 2021

Something like ::test::fail() to fail the current test without panicking would help, that that requires support from the standard library.

Yeah we can probably just add that to the standard library. :)

The alternative is for assert2 to provide its own #[test] macro, I suppose.

@de-vri-es
Copy link
Owner

Right, that would be the best solution I think. Returning a boolean or other object that you can forget to use sounds like a potential footgun.

Although multithreading and async code may make ::test::fail() difficult to implement in practice. Although panics in background threads don't fail test now either, I suppose?

@m-ou-se
Copy link
Collaborator

m-ou-se commented Oct 11, 2021

Although multithreading and async code may make ::test::fail() difficult to implement in practice.

Yes, we need inheritable thread locals. Would be cool to have those.

Although panics in background threads don't fail test now either, I suppose?

Yeah, but those are usually 'forwarded' to the main thread by thread.join().unwrap(). (In one-process-per-test mode with panic=abort, panics anywhere work to fail a test though.)


Alternatively, assert2 could have something like a check_scope! { .. }, that will panic at the end if any check inside failled. Makes things a bit more flexible too.

@de-vri-es
Copy link
Owner

de-vri-es commented Oct 11, 2021

I think check_scope! would be hard to implement correctly for asynchronous code. Relying on thread locals would not work correctly when multiple futures are interleaved on the same thread. This is also why I haven't added message!() and capture!() yet :(

Having check_scope! { } recognize check!() doesn't feel right either: apart from some implementation difficulties, it wouldn't work with check!() inside function calls.

@DanielJoyce
Copy link

You could have a version of check that returned the scope guard so then you could decide when to drop it?

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

No branches or pull requests

4 participants