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

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

Merged
merged 2 commits into from
Sep 12, 2015

Conversation

IainNZ
Copy link
Member

@IainNZ IainNZ commented Sep 12, 2015

  • 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

quinnj and others added 2 commits September 12, 2015 11:59
…ith them. Fixes issues on windows where OS complains when trying to mmap files that have already been mmapped/opened
@IainNZ IainNZ added the test This change adds or pertains to unit tests label Sep 12, 2015
@tkelman
Copy link
Contributor

tkelman commented Sep 12, 2015

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
Copy link
Contributor

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

@tkelman
Copy link
Contributor

tkelman commented Sep 12, 2015

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

@yuyichao
Copy link
Contributor

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
Copy link
Contributor

tkelman commented Sep 12, 2015

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
Copy link
Contributor

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
Copy link
Member Author

IainNZ commented Sep 12, 2015

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
Copy link
Contributor

tkelman commented Sep 12, 2015

I think that this is a bad pattern,

Could you elaborate why?

@yuyichao
Copy link
Contributor

I think that this is a bad pattern

So what would be a good pattern?

@yuyichao yuyichao closed this Sep 12, 2015
@yuyichao yuyichao reopened this Sep 12, 2015
@yuyichao
Copy link
Contributor

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

@IainNZ
Copy link
Member Author

IainNZ commented Sep 12, 2015

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
Copy link
Contributor

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
Copy link
Member Author

IainNZ commented Sep 12, 2015

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
Copy link
Contributor

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
Copy link
Contributor

ssfrr commented Sep 12, 2015

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
Copy link
Member Author

IainNZ commented Sep 12, 2015

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 IainNZ changed the title Tweaks to tests Tweaks to tests (mmap robustness, don't use at-test_throw return type) Sep 12, 2015
@IainNZ
Copy link
Member Author

IainNZ commented Sep 12, 2015

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

@tkelman
Copy link
Contributor

tkelman commented Sep 12, 2015

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
Copy link
Member Author

IainNZ commented Sep 12, 2015

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
Copy link
Contributor

relying on internal fields like this

Than add an function to access it?

@IainNZ
Copy link
Member Author

IainNZ commented Sep 12, 2015

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
Copy link
Member Author

IainNZ commented Sep 12, 2015

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
Copy link
Contributor

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

@IainNZ
Copy link
Member Author

IainNZ commented Sep 12, 2015

This PR covers all uses in Base.Test

@IainNZ
Copy link
Member Author

IainNZ commented Sep 12, 2015

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

@IainNZ
Copy link
Member Author

IainNZ commented Sep 12, 2015

Seems everyone ended up being satisfied with this

IainNZ added a commit that referenced this pull request Sep 12, 2015
Tweaks to tests (mmap robustness, don't use at-test_throw return type)
@IainNZ IainNZ merged commit 4a2298d into master Sep 12, 2015
@IainNZ IainNZ deleted the ird/testref branch September 12, 2015 22:49
quinnj added a commit that referenced this pull request Sep 13, 2015
…ith them. Fixes issues on windows where OS complains when trying to mmap files that have already been mmapped/opened

(cherry picked from commit e8953fc)
ref #13090
@tkelman
Copy link
Contributor

tkelman commented Sep 16, 2015

backported in #13107

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants