Change @test ex broken=true to error if ex evaluates to non-boolean
#47804
Conversation
Just like `@test ex` errors if `ex` does not evaluable to a boolean, the `broken=true` variant should as well. As it stands, right now the following does not result in an error: ``` @test 1 broken=true ``` While the following does result in an error: ``` @test 1 ``` The intention of `broken=true` is to allow for tests to be added such that they pass although something is broken, and then fail once the missing functionality has been added, so that the developer knows to go and adjust the tests for the newly-added functionality. It is often used in situations such as: ``` @test partially_implemented_test() broken=Sys.iswindows() ``` Sometimes, the function under test throws an error in certain situations, and this test can even be used to track a function erroring out, and notifying the developer when it no longer throws: ``` @test this_throws() broken=true ``` However, this occasionally leads to improper usage such as: ``` @test this_usually_returns_an_object_but_sometimes_throws() broken=true ``` This test, when it throws, passes (because of `broken=true`) but then when it returns a non-boolean, it _also_ passes. This is Very Bad (TM). This PR fixes the state of affairs by making a non-boolean return an error here.
|
The current behaviour matches which is what I don't have a strong opinion against this change at the moment, but I wonder if that breaks someone's workflow (especially in |
They are both macroexpanded to the same underlying code (
Errors are still ignored! It's just a non-erroring result that does not return a boolean that is under consideration here, because if you have a test that looks like: where |
There was a problem hiding this comment.
Right, I misread the code, I thought this was affecting only the @test macro!
I wanted to suggest to add a test, but it seems a bit tricky, something like
julia> @testset begin
@test_throws Test.FallbackTestSetException @test(1)
end;
test set: Error During Test at REPL[21]:2
Expression evaluated to non-Boolean
Expression: 1
Value: 1
test set: Test Failed at REPL[21]:2
Expression: #= REPL[21]:2 =# @test 1
Expected: Test.FallbackTestSetException
No exception thrown
Stacktrace:
[1] macro expansion
@ REPL[21]:2 [inlined]
[2] macro expansion
@ ~/.julia/juliaup/julia-1.8.2+0.x64/share/julia/stdlib/v1.8/Test/src/Test.jl:1357 [inlined]
[3] top-level scope
@ REPL[21]:2
Test Summary: | Fail Error Total Time
test set | 1 1 2 0.3s
ERROR: Some tests did not pass: 0 passed, 1 failed, 1 errored, 0 broken.
counts as a failing test 😞
Edit: I think I botched the @test_throws anyway, but I didn't manage to catch the error without failing the test with some quick tests (too many "tests")
|
Sounds like this uncovered some problems ? 😃 |
This test does not fail on any Julia version I've tested going all the way back to v0.7 when it was first introduced. My assumption is that it broke on an RC version, and we didn't noticed when it was fixed because `@test_broken` ignored tests that returned non-boolean values.
This test is still valid, but it was not actually asserting anything due to not returning a boolean value.
staticfloat
force-pushed
the
sf/broken_test_nonboolean
branch
from
564f7a7 to
508bb92
Compare
This `@test_broken` is meant to just register a "broken" result for these platforms, so let's ensure the test always results in a `false`.
|
Mosè is right, this already found three examples of this breakage in the test suite! |
Just like
@test exerrors ifexdoes not evaluable to a boolean, thebroken=truevariant should as well. As it stands, right now the following does not result in an error:While the following does result in an error:
The intention of
broken=trueis to allow for tests to be added such that they pass although something is broken, and then fail once the missing functionality has been added, so that the developer knows to go and adjust the tests for the newly-added functionality. It is often used in situations such as:Sometimes, the function under test throws an error in certain situations, and this test can even be used to track a function erroring out, and notifying the developer when it no longer throws:
However, this occasionally leads to improper usage such as:
This test, when it throws, passes (because of
broken=true) but then when it returns a non-boolean, it also passes. This is Very Bad (TM). This PR fixes the state of affairs by making a non-boolean return an error here.