Replacement for Base.Test (ready for review) #13062

IainNZ · 2015-09-11T02:53:44Z

The goal of this PR is to make Base.Test better than just assert with some fancier output. It does so primarily by introducing @testset, which lets users bundle tests together and delay throwing an error until the end.

Design-wise, it primarily needs to be non-breaking for 99.9% uses of the current Base.Test, and be simple, minimal, and relatively opinion-free. It is clear there is a large space of possibilities for testing frameworks, but I don't want to go there (personally). My goal is for a incremental improvement over the existing Base.Test - if someone wants to pick up the ball and add more features later, they can. A thing that falls outside the scope includes fancier outputs (to, e.g. files).

This PR is basically https://github.com/IainNZ/BaseTestNext.jl. It is a drop-in replacement for Base.Test, and all tests except for those in test/test.jl pass without modification.

Features for later PRs (probably not by me):

Use the special formatting on Travis for expand/collapse (via @simonbyrne)
Enable changing the default test set used, which then enables custom output (e.g. JSON, XML)
@test_approx needs to be either included in the testset world, or even better deprecated in favor of isapprox based testing.

Some demo output from BaseTestNext.jl

jakebolewski · 2015-09-11T03:00:43Z

💯

For Travis esp for the standard library tests I think another Todo would be collapsed output. If all the tests within a testfile pass then a summary is printed instead of the detailed output. The detailed output is only expanded when one or more tests fail within a given file.

IainNZ · 2015-09-11T06:02:21Z

@tkelman any ideas why mmap is dying on Windows? As far as I can, its the only thing this PR breaks, but the tests for mmap are somewhat impenetrable to me. The line in the backtrace is particularly as there isn't even a test there!

tkelman · 2015-09-11T06:11:51Z

test/core.jl

@@ -3100,15 +3100,25 @@ Base.convert(::Type{Foo11874},x::Int) = float(x)

 # issue #9233
 let
-    err = @test_throws TypeError NTuple{Int, 1}


you can't make @test_throws return the error?

It'd be inconsistent with @test. Now everything returns a test object, with nice behaviour at the REPL as a result. I think using @test_throws as a one-liner try-catch was not a good idea to begin with, although thats completed unrelated to why I picked the return type.

okay fair enough, sounds like there's a good reason for the change. slightly breaking if any packages were doing this, but probably worth it

aviks · 2015-09-11T06:55:08Z

+100. This is the one thing I've wanted as far as incremental improvements to Base.Test go.

ScottPJones · 2015-09-11T14:51:07Z

👍 💯 I'd lobbied for just this sort of thing a while ago, great to see something like this actually being built.

tbreloff · 2015-09-11T15:08:45Z

This is exciting... do you see it making FactCheck.jl obsolete at some
point?

On Fri, Sep 11, 2015 at 10:51 AM, Scott P. Jones [email protected]
wrote:

[image: 👍] [image: 💯] I'd lobbied for just this sort of thing a
while ago, great to see something like this actually being built.

—
Reply to this email directly or view it on GitHub
#13062 (comment).

IainNZ · 2015-09-11T15:18:50Z

@tbreloff it makes FactCheck obsolete for my personal needs, and I won't be working on it any more, but as a package it is always going to have much more flexibility in how it does things because it can be quite opinionated/crazy if it wants.

quinnj · 2015-09-11T16:01:30Z

@IainNZ, my guess is the mmap failure probably has to do with trying to mmap an open file, which results in the invalid argument. We may need to explicitly call finalize more on the mmapp-ed arrays to ensure the files are getting garbage collected.

IainNZ · 2015-09-11T16:07:21Z

@quinnj Yes that makes sense. This shouldn't be keeping any references around any longer than the old @test did, but it would probably change the GC pattern enough to reveal something. If you have any ideas on how to fix (or make the tests more robust), I'm all ears!

quinnj · 2015-09-11T16:36:04Z

Just pushed some changes that make mmap tests pass on my windows machine.

quinnj · 2015-09-11T16:36:11Z

(to this PR)

IainNZ · 2015-09-11T18:05:53Z

Hah, well I'll be damned, very nice work.
I'm going to rebase the commits to put that first, then the rest of it on top of that so we have a passing history.

IainNZ · 2015-09-11T18:10:26Z

Ok, lets see how CI likes that

ssfrr · 2015-09-12T16:54:41Z

To quote from the #13090:

with @test_throws returning an exception:

err = @test_throws TypeError NTuple{Int, 1}
@test err.func == :NTuple
@test err.expected == Int
@test err.got == Int

becomes (if @test_throws doesn't return the exception):

try
    NTuple{Int, 1}
    @test false
catch err
    @test isa(err, TypeError)
    @test err.func == :NTuple
    @test err.expected == Int
    @test err.got == Int
end

I think the first is more clear, and the extra try...catch stuff sticks out even more if you're only checking one property of the exception (say err.msg). I really like @tkelman's idea of attaching the exception to the Result, seems like the best of both worlds.

IainNZ · 2015-09-12T16:59:37Z

I don't think the first is more clear, I think its using @test_throws for more than its intended. But returned the exception as part of the result type is fine, and I'll tweak it so it does (it currently only does so if the wrong test type was thrown, so its a 5 character change).

IainNZ · 2015-09-12T17:26:58Z

OK, now returns the exception value always (not just on failure)

IainNZ · 2015-09-12T23:06:57Z

Now #13090 is in, rebased.

milktrader · 2015-09-12T23:54:00Z

Cool work @IainNZ

tkelman · 2015-09-13T16:21:01Z

I guess anywhere in the testing docs that mentions handlers should probably be updated here since that mechanism is being removed.

IainNZ · 2015-09-13T16:29:16Z

Yeah I pulled that out of the helpdb thing to get it to compile, but I need to substantially rewrite that chapter before merging this.

IainNZ · 2015-09-15T04:07:54Z

So I'm trying to do something sensible with test_approx_eq, but they don't really work like any of the rest of the tests anyway. I noticed that

macro test_approx_eq_eps(a, b, c)
    ex = :(isapprox($a, $b, atol=$c))
    :(do_test(()->($(Expr(:quote,ex)), $(esc(ex))), $(Expr(:quote,ex))))
end

actually passes all the tests in base, but that just might be because its easier to satisfy than the current criteria (it is essentially maxabs now, I think, and isapprox does vecnorm).

Is punting on "integrating" these into the new framework a viable option? I'm running out of time and steam for this PR. My preferred option would be to actually deprecate them in favour of using @test and isapprox, but thats something I don't have the resources to make happen (in Base, at least). Maybe we could recommend it for packages though.

StefanKarpinski · 2015-09-15T22:51:01Z

Yes, punting on these is very reasonable. I'd love to get rid of them altogether. They should be normal tests with a different predicate, not some weird special testing macro.

stevengj · 2015-09-17T00:10:12Z

@IainNZ, if you want to use the more-stringent maxabs check, you can pass norm=maxabs (or norm=x->vecnorm(x,Inf)) to isapprox. Though maybe isapprox will need to be modified to take a norm keyword argument in the scalar case first.

But I agree with @StefanKarpinski that it would be better to get rid of those special test macros eventually.

ssfrr · 2015-09-21T04:03:25Z

Is it expected that passed testsets are displayed if their sibling fails? e.g.

@testset "outer" begin # should be displayed
    @testset "inner 1" begin # should not be displayed?
        @test true
    end
    @testset "inner 2" begin # should be displayed
        @test false
    end
end

gives

inner 2: Test Failed
  Expression: false
   Evaluated: false
 in record at /Users/srussell/.julia/v0.4/BaseTestNext/src/defaulttestset.jl:24
 in do_test at /Users/srussell/.julia/v0.4/BaseTestNext/src/BaseTestNext.jl:188
Test Summary: | Pass  Fail  Total
outer         |    1     1      2
  inner 1     |    1            1
  inner 2     |          1      1
ERROR: Some tests did not pass: 1 passed, 1 failed, 0 errored.
 in finish at /Users/srussell/.julia/v0.4/BaseTestNext/src/defaulttestset.jl:89

I don't really have a preference one way or the other (it could provide some extra context), but the docs say:

In the event that a nested test set has no failures, as happened here, it will be hidden in the summary. If we do have a test failure, only the details for the failed test sets will be shown.

which makes it sound like it might be a bug.

ssfrr · 2015-09-21T04:08:54Z

BTW, I don't think that has to be a blocking issue even if it's not intended behavior. I know you're trying to tie a bow on this thing so if it merges as-is and we file an issue, I can look into it. Or just change the docs. :)

Replacement for Base.Test (ready for review)

KristofferC · 2015-09-21T11:23:42Z

Great!

stevengj · 2015-09-21T15:28:59Z

Needs a 0.5 NEWS entry...

IainNZ · 2015-09-21T15:44:46Z

Is backporting to 0.4 viable?

IainNZ · 2015-09-21T15:46:20Z

@ssfrr it is intentional, I guess I just view it differently. So if a testset has a failure, then that failure is also associated with its parent. If I hid the "sibling" test sets it looks a bit confusing (especially when their a multiple failures), because the "children" don't sum up to the parent's totals

stevengj · 2015-09-21T15:51:09Z

Backporting a new feature like this doesn't make sense to me.

IainNZ · 2015-09-21T16:00:05Z

Not even to 0.4.1?

bicycle1885 · 2015-09-21T16:05:54Z

Is that possible to provide this as a separated package (or Compat.jl)? I really want to use it!

StefanKarpinski · 2015-09-21T16:07:30Z

It's a big feature, but not really a normal language feature. The question is what breaks if we change this?

IainNZ · 2015-09-21T16:09:56Z

Anything using with_handler breaks. It wasn't very widely used, but according to
https://github.com/search?l=julia&q=with_handler&type=Code&utf8=%E2%9C%93
some people did use it.

I could maybe add deprecation machinery for it?

IainNZ · 2015-09-21T16:14:16Z

(it was mostly used for lack of an easy way to not stop on the first failing test, and a lot of the results in that search are forks of Julia)

StefanKarpinski · 2015-09-21T16:17:46Z

We can also still technically sneak features into 0.4 since it's not final yet. @IainNZ, how painful will maintaining support for two different Base testing APIs be?

IainNZ · 2015-09-21T16:20:24Z

I'll investigate today, I'm guessing not terrible.

tkelman · 2015-09-21T16:44:04Z

I'm very much against backporting anything into release-0.4 right now that isnt a critical bugfix or absolutely harmless (like doc additions). We've done 2 rc's already, now's not the time. Maybe for 0.4.1.

StefanKarpinski · 2015-09-21T16:47:42Z

Well, 0.4.1 isn't the time since technically this is a breaking change and that would violate our principle that your code should not break between 0.4.0 and 0.4.1; otoh, there's no such promise going from 0.4.0-rc2 to 0.4.0.

tkelman · 2015-09-21T16:59:27Z

We're trying to get a release out that's way behind schedule. This needs people testing and using it, it is several hundred lines of code after all. We're technically being stricter than semver since we're pre 1.0. The least risky way of supporting this on 0.4 is as a package.

mbauman · 2015-09-21T17:00:39Z

Can the changes to with_handler be made nonbreaking? Then it could be introduced purely as a feature for 0.4.1. Still not ideal to introduce features in patch releases, but I think it'd be ok (especially since it'll have had a month or two of testing on master).

ssfrr · 2015-09-21T17:12:56Z

the other breaking change is the return value of @test_throws, which now returns a Result instead of the raw exception.

IainNZ · 2015-09-21T17:31:44Z

@ssfrr that isn't a breaking change anymore, as it turned out that feature was added in 0.4 and has since been removed by this PR.

@mbauman probably.

My initial plan was to have this is a package for all of 0.4 anyway, so getting it into 0.4.0 isn't personally critical to me.

You should check for the version where JuliaLang/julia#13062 was merged.

…as merged

IainNZ added this to the 0.5.0 milestone Sep 11, 2015

IainNZ added the test This change adds or pertains to unit tests label Sep 11, 2015

IainNZ force-pushed the ird/testnext branch from 2dfb5ef to 8aca6fc Compare September 11, 2015 03:19

tkelman reviewed Sep 11, 2015
View reviewed changes

IainNZ force-pushed the ird/testnext branch from 698d920 to b44700b Compare September 11, 2015 18:10

IainNZ mentioned this pull request Sep 12, 2015

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

Merged

IainNZ force-pushed the ird/testnext branch from b44700b to 8998a8b Compare September 12, 2015 17:25

IainNZ force-pushed the ird/testnext branch from 8998a8b to 613b0fd Compare September 12, 2015 22:51

jakebolewski added a commit that referenced this pull request Sep 21, 2015

Merge pull request #13062 from JuliaLang/ird/testnext

b1e447f

Replacement for Base.Test (ready for review)

jakebolewski merged commit b1e447f into master Sep 21, 2015

yuyichao deleted the ird/testnext branch September 21, 2015 11:28

ihnorton mentioned this pull request Sep 23, 2015

Don't bail out on test errors by default #10835

Closed

ihnorton mentioned this pull request Oct 8, 2015

Text says AST, example shows backtrace. #8258

Closed

KristofferC mentioned this pull request Nov 1, 2015

drop package dependencies JuliaFEM/JuliaFEM.jl#66

Closed

stevengj added a commit to stevengj/BaseTestNext.jl that referenced this pull request Dec 9, 2015

use correct 0.5 dev version in README

bb72143

You should check for the version where JuliaLang/julia#13062 was merged.

stevengj mentioned this pull request Dec 9, 2015

use correct 0.5 dev version in README JuliaCI/BaseTestNext.jl#8

Merged

stevengj added a commit to stevengj/PETSc.jl that referenced this pull request Dec 9, 2015

use BaseTestNext before 0.5-dev version where JuliaLang/julia#13062 w…

843366a

…as merged

s-celles mentioned this pull request Nov 12, 2017

Adopt @testset syntax for PyTest.jl pdobacz/PyTest.jl#1

Open

Replacement for Base.Test (ready for review) #13062

Replacement for Base.Test (ready for review) #13062

Conversation

IainNZ commented Sep 11, 2015

jakebolewski commented Sep 11, 2015

IainNZ commented Sep 11, 2015

tkelman Sep 11, 2015

Choose a reason for hiding this comment

IainNZ Sep 11, 2015

Choose a reason for hiding this comment

tkelman Sep 11, 2015

Choose a reason for hiding this comment

aviks commented Sep 11, 2015

ScottPJones commented Sep 11, 2015

tbreloff commented Sep 11, 2015

IainNZ commented Sep 11, 2015

quinnj commented Sep 11, 2015

IainNZ commented Sep 11, 2015

quinnj commented Sep 11, 2015

quinnj commented Sep 11, 2015

IainNZ commented Sep 11, 2015

IainNZ commented Sep 11, 2015

ssfrr commented Sep 12, 2015

IainNZ commented Sep 12, 2015

IainNZ commented Sep 12, 2015

IainNZ commented Sep 12, 2015

milktrader commented Sep 12, 2015

tkelman commented Sep 13, 2015

IainNZ commented Sep 13, 2015

IainNZ commented Sep 15, 2015

StefanKarpinski commented Sep 15, 2015

stevengj commented Sep 17, 2015

ssfrr commented Sep 21, 2015

ssfrr commented Sep 21, 2015

KristofferC commented Sep 21, 2015

stevengj commented Sep 21, 2015

IainNZ commented Sep 21, 2015

IainNZ commented Sep 21, 2015

stevengj commented Sep 21, 2015

IainNZ commented Sep 21, 2015

bicycle1885 commented Sep 21, 2015

StefanKarpinski commented Sep 21, 2015

IainNZ commented Sep 21, 2015

IainNZ commented Sep 21, 2015

StefanKarpinski commented Sep 21, 2015

IainNZ commented Sep 21, 2015

tkelman commented Sep 21, 2015

StefanKarpinski commented Sep 21, 2015

tkelman commented Sep 21, 2015

mbauman commented Sep 21, 2015

ssfrr commented Sep 21, 2015

IainNZ commented Sep 21, 2015