Use test_throws instead of test_broken in Float16 numbers tests #17396
Conversation
| @@ -2771,7 +2771,8 @@ let types = (Base.BitInteger_types..., BigInt, Bool, | |||
| for S in types | |||
| for op in (+, -) | |||
| if S === Float16 # type instability here? | |||
| @test_broken @inferred Base.promote_op(op, S) | |||
| # broken, fixme then remove this branch | |||
There was a problem hiding this comment.
I think the whole point of @test_broken is that it can show up in the report as broken. Since it doesn't work for this, it's better to just disable the test (i.e. remove the @inferred) and leave a comment. @test_throws is almost certainly the wrong thing to do.
There was a problem hiding this comment.
Maybe we should change @test_broken to allow the argument to either fail or throw. Or introduce another macro @test_broken_throws, although in that case I'd be tempted to rename these macros as @broken_test and @broken_test_throws... Or maybe better still spell them as @broken @test and @broken @test_throws, etc.
There was a problem hiding this comment.
The issue is that @test_broken is currently a replacement of @test. So if @test foo is broken, you replace it with @test_broken foo. However, here @inferred is the @test so using @test_broken or @test_throws like this is effectively writing @test_throws @test or @test_broken @test. I think we could possibly make this a valid use of @test_broken but this is almost certainly not a valid use of @test_throws.
There was a problem hiding this comment.
why is test_throws wrong? it achieves the test_broken purpose of being noisy and loud and making you fix it if the test changes result. we don't have a report right now so the count of broken tests isn't visible anywhere except via grep
There was a problem hiding this comment.
It's wrong in the same way using @test !foo instead of @test_broken foo is wrong.
There was a problem hiding this comment.
Before we had @test_broken that wasn't all that wrong, for making more-visible fixme's.
And neither of those deal with exceptions (test_broken allows them, but doesn't tell you if something changes result from exception to non-boolean), @test_throws does.
There was a problem hiding this comment.
Before we had @test_broken the right way was commenting the test out. Now we have @test_broken the right way is to make @test_broken @inferred works.
There was a problem hiding this comment.
No, commenting the test out doesn't tell you when you fixed the behavior - that's much more of the point of what @test_broken is useful for. Yes, rewriting @test_broken @inferred to do the right thing would be a good idea, but is also more messing with macros than I have the patience for right now.
There was a problem hiding this comment.
... OK apparently @inferred is not even a test. In that case I'm fine with using @test_throws for now. Note that using this on a test will mess things up.
julia> @test_throws ErrorException @test false
Test Failed
Expression: false
Test Passed
Expression: @test false
Thrown: ErrorExceptionThere was a problem hiding this comment.
yeah @inferred isn't a first-class member of the test system in terms of behavior or return types, it's a separate mostly-independent utility
Since test_broken is not good for things that currently error but when fixed will return non-boolean.
If you know how to fix the inference issue and get tests to pass with #17394 (377dede) reverted, then please do so and close this PR.