Tweaks to tests (mmap robustness, don't use at-test_throw return type)

[IainNZ]

• First commit by @quinnj robustifies the mmap tests, which avoided an issue on Windows.
• Second commit modifies some tests to use try-catch instead of relying on @test_throws to return the thrown exception.

Both these came out of #13062 (the first is cherry picked, the second manually extracted).

Ping: @tkelman @StefanKarpinski

[tkelman]

I'm grepping through registered packages looking for anyone using the return value of @test_throws and having a hard time finding anything. So maybe we do want to pre-emptively stop doing that to make backporting #13062 more feasible on some later point release of 0.4.

[yuyichao]

Why is it a bad idea to let @test_throws returns the exception thrown?

[tkelman]

Rarely used feature, could instead be used for more general and uniform test result encapsulation, display, summaries, etc?

[yuyichao]

having a hard time finding anything

It's expected since this is added during 0.4.

The reason I added the return value to @test_throws is that I think @test_throws (or a similar macro) should be used to make it easier to test for the properties of the exception thrown.

It seems wrong to me that whenever you want to test for anything other than the type (e.g. in order to make sure that the error is thrown at the expected place/function and has the necessary information included) you would have to go back to write the try-catch manually.

I'm not really familiar with FactCheck but I don't think returning a value in @test_thrown will conflict with that style. You can still add @fact that check other properties on the return value and it is not very different from checking the return value of any other function.

That said I'm also totally fine with extending @test_throws to allow specifying other properties of the exception. Maybe a similar syntax with how the conditions are specified in FactCheck. If it is planned to add these functions, I agree it is not a big deal to remove the return value for now.

[tkelman]

This is more a question for #13062 unless/until we also change the behavior of @test_throws instead of just the usage here. But couldn't we make @test_throws return a more general object that stores the exception, along with other information?

[yuyichao]

Rarely used feature

Mostly because it is new...

could instead be used for more general and uniform test result encapsulation, display, summaries, etc?

Sounds reasonable. Would still be nice to make it easier to test for other properties of the exception. Doesn't have to be using the return value and doesn't have to be directly the return value though.

[IainNZ]

In the new proposal, it'd return a more general Result object, that would contain the exception thrown. I think that this is a bad pattern, but if you still want to do it, you could.

[tkelman]

I think that this is a bad pattern,

Could you elaborate why?

[yuyichao]

I think that this is a bad pattern

So what would be a good pattern?

[yuyichao]

................................................ Sorry I meant to cancel my comment which is redundant with Tony's....

[IainNZ]

Why @test_throws directly returning the expression? Because @test* methods shouldn't always immediately throw, otherwise why not just use an assert? If they shouldn't immediately throw, they should return some object that describes what the test was and what happen. Then you can chose to throw.

ex = @test_throws FooError foo()

Could error because foo doesn't throw anything, could error because foo throws a BarError instead, etc. So it seems like you'd want to be able to check why its throwing.

Or for why using the Result type? Its not as bad, but I still think try catch is more explicit, and doesn't require digging into internal fields of Base.Test stuff.

[yuyichao]

I think the necessary logic here is that if the expected exception type is returned, it might need to do some extra check on the exception. With return value I think what could be done is

test_result = @test_throws FooError foo()
if test_result.pass
     check_exception_thrown(test_result.exception)
else
    # In case you want to print sth to help figuring out what might be wrong
end

[IainNZ]

Here is how it works with the revised Base.Test

If not in a @testset:

res = @test_throws FooError foo()
if isa(res, Test.Pass)
  ex = res.value
  # Do your stuff
else
  # Won't reach here, because it will have thrown already
  # This is the current behaviour
end

If in a @testset:

@testset begin
  res = @test_throws FooError foo()
  if isa(res, Test.Pass)
    ex = res.value
    # Do your stuff
  else
    # Do other stuff?
  end
end # throws TestSetException now

[yuyichao]

I thought you mean including the error thrown (res.value) in the test pass case for @test_throws is a bad style.

The example you have above looks good to me. (And I guess for the not in @testset case the if won't be necessary and the only different from now is to write res.value instead of res)

[ssfrr]

Oh, I see your point now. I'm used to test frameworks where a test case bails out on the first failed @test*, which I see now isn't the case. In those systems, at a given point in your test case you can assume that previous tests have passed. The reason to use @test rather than @assert in those cases is that you might want to still keep running your test suite and get info about failures. I don't think that one or the other approach is necessarily better, but I was using the wrong mental model thinking about this problem.

Given that execution continues after the @test_throws even if it doesn't throw, I agree that the try...catch pattern is the right way to test properties of the exception.

[IainNZ]

Yep its just a change to res.value. It also means if you do @test_throw at the REPL, you get an informative response.

[IainNZ]

So this returning the exception thing was added in 0.4? That means backporting is even more reasonable then, right @tkelman ?

[tkelman]

That explains why it's not very widely used yet. We'd still be breaking it (putting in a slightly different but more general API instead) with a backport if it survives into 0.4.0 final, do we want to pre-emptively do anything about that now or not?

[IainNZ]

The relevant line currently is default_handler(r::Success) = r.res. Even if we changed that to = r, I've still changed the field name to value. I could, of course, just also use .res, but it all smells funny to me relying on internal fields like this (part my dislike for the whole pattern).

[yuyichao]

relying on internal fields like this

Than add an function to access it?

[IainNZ]

Its a function with literally one use though, extract the value of a @test_throws test. You wouldn't want the value of any other test - they just return true or false.

[IainNZ]

I'd rather just un-document that The default handler returns the exception if it is of the expected type., thus making the return type not guaranteed.

[yuyichao]

I think it's fine to just revert #11984 and any other current use of it in the base test.

[IainNZ]

This PR covers all uses in Base.Test

[IainNZ]

So we're OK to merge this change first then? Any objections?

[IainNZ]

Seems everyone ended up being satisfied with this

[tkelman]

backported in #13107