Change @test ex broken=true to error if ex evaluates to non-boolean

[staticfloat]

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.

[giordano]

The current behaviour matches

julia> @test_broken 1
Test Broken
  Expression: 1

which is what @test ... broken=true was modelled on. Maybe, also @test_broken should be changed in the same way? Otherwise they'd diverge.

I don't have a strong opinion against this change at the moment, but I wonder if that breaks someone's workflow (especially in @test_broken, which is much more widely used) because I kinda feel like it can be useful to ignore errors under broken=true 🤔

[staticfloat]

Maybe, also @test_broken should be changed in the same way?

They are both macroexpanded to the same underlying code (do_broken_test), so it gets changed in the same way, for free:

julia> using Test
       @test_broken 1
Error During Test at REPL[2]:2
  Expression evaluated to non-Boolean
  Expression: 1
       Value: 1
ERROR: There was an error during testing

because I kinda feel like it can be useful to ignore errors unde

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:

@test foo() broken=true`

where foo() errors, you get the expected result. But then later, when you fix foo() and it returns, say, 4.0, this test should start to fail, but it doesn't, because it only currently fails if foo() returns true.

[giordano]

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")

[giordano]

Sounds like this uncovered some problems

Error in testset subtype:
Error During Test at /cache/build/default-amdci4-4/julialang/julia-master/julia-92bb4f09c8/share/julia/test/subtype.jl:2233
  Expression evaluated to non-Boolean
  Expression: typeintersect(Type{Tuple{Array{T, 1} where T}}, UnionAll)
       Value: Union{}
Error in testset sets:
Error During Test at /cache/build/default-amdci4-4/julialang/julia-master/julia-92bb4f09c8/share/julia/test/sets.jl:737
  Expression evaluated to non-Boolean
  Expression: #= /cache/build/default-amdci4-4/julialang/julia-master/julia-92bb4f09c8/share/julia/test/sets.jl:737 =# @inferred replace([1, missing], missing => 2)
       Value: [1, 2]

? 😃

[staticfloat]

Mosè is right, this already found three examples of this breakage in the test suite!