[WIP] Help wanted: switch Base.Test to use testsets everywhere #17165

kshyatt · 2016-06-28T17:01:42Z

We have all this great testing infrastructure we're not using. I changed test/runtests.jl to collect @testset results from files and generate a nice report at the end, also using @timed for more statistics (gc time!) instead of @elapsed. But we have some problems:

not sure how to work with parallel{_exec}.jl and boundscheck{_exec}.jl - need to pass testset results along from the exec files.
test/test.jl is broken because of changes I made!
Make printing more attractive - somewhat done, now sending error and failure info to STDERR which it seems to me we should have been doing all along. Need to print error backtraces still. Should we align the gc time/memory info I have?
Tests may be much slower now
Make sure that packages can use this as a drop-in no-effort replacement
print the error backtraces right
return max_rss as part of the tuple in runtests
the build needs to not time out/segfault

I would really appreciate help with any/all of this. cc: @ranjanan

StefanKarpinski · 2016-06-28T17:06:08Z

This is really nice. The test output is even nicer than the way you write the tests.

KristofferC · 2016-06-28T17:10:34Z

I thought the reason we didn't use testset is because the output was a bit too verbose. Did something change regarding this? @tkelman

kshyatt · 2016-06-28T17:12:20Z

@KristofferC the output doesn't have to be verbose if you nest your @testsets properly, which we can & will modify the files in test to do. I'll post a screenshot example or you can checkout the branch, deliberately mess up a test in test/math.jl, and run make test-math to see what happens.

StefanKarpinski · 2016-06-28T17:12:21Z

Yes, @kshyatt did a bunch of work to make the output look nice.

quinnj · 2016-06-28T17:33:26Z

Why would existing tests "not work"? (i.e dates/ranges.jl?)

kshyatt · 2016-06-28T17:43:24Z

@quinnj Wrapping dates/ranges in a @testset is making a bunch of the tests fail.

Specifically:

@test length(a:Dates.Year(1):Dates.Date(2020,2,1)) == 8
@test length(a:Dates.Year(1):Dates.Date(2020,6,1)) == 8
@test length(a:Dates.Year(1):Dates.Date(2020,11,1)) == 8
@test length(a:Dates.Year(1):Dates.Date(2020,12,31)) == 8
@test length(a:Dates.Year(1):Dates.Date(2021,1,1)) == 9

Don't work.

ranjanan · 2016-06-28T19:11:13Z

@kshyatt I get an error in dates/arithmetic.jl at

@test (Date(2009,1,1):Week(1):Date(2009,1,21)) - (Date(2009,1,1):Day(1):Date(2009,1,3)) == [0d, 6d, 12d]

with UndefVarError for d

kshyatt · 2016-06-28T19:24:30Z

@ranjanan Me too, I'm fixing it now. linalg/arnoldi is also broken.

tkelman · 2016-06-28T20:56:48Z

It seems like the top-level testset per file could probably just be figured out from the filename automatically, either in Base.Test or test/runtests.jl, instead of included by hand?

kshyatt · 2016-06-28T21:14:40Z

It seems like the top-level testset per file could probably just be figured out from the filename automatically, either in Base.Test or test/runtests.jl, instead of included by hand?

This might be annoying for the subdirectories like linalg, strings, etc. though

ranjanan · 2016-06-28T21:16:20Z

@kshyatt #17171 fixes dates/arithmetic.jl tests

kshyatt · 2016-06-28T21:22:04Z

OK I've updated the PR with some fixes. I'll cherry-pick @ranjanan's fixes as well in a bit.

tkelman · 2016-06-28T21:24:32Z

for the subdirectories

That would be a good reason to do the nice version in Base.Test - maybe @testset include could be rewritten to this?

StefanKarpinski · 2016-06-28T21:42:12Z

Let's keep it simple for now. We can refine the mechanisms later.

kshyatt · 2016-07-02T17:30:46Z

I've updated this PR a bunch! @StefanKarpinski @tkelman @ranjanan thoughts? @carnaval is the gc info we are printing useful to you?

kshyatt · 2016-07-02T18:20:49Z

The top-level modules in enums and docs are breaking on remote workers with an UndefVarError - anyone know why?

IainNZ · 2016-07-02T18:35:54Z

base/test.jl

    # Finally throw an error as we are the outermost test set
-    if total != total_pass
+    #print_test_results(ts)


Whats happening here now?

I changed things so that now only the King of the Nodes outputs anything. Otherwise we were getting horrendous looking spam from each worker.

So am I reading this right: if I'm just a general package, and I'm using Base.Test, there will be no output from test sets?

You can dump it explicitly with Base.Test.print_test_results(Base.Test.get_testset()) but yeah, it would be silent unless things fail. There's a reason I put WIP on this. I wish I could put bold in titles: WIP. Ideally I think we'd have a verbose/quiet flag you could pass depending if you want output or not. It would be good to just let packages use test/runtests.jl themselves, rather than hacking their own weird fake parallel testing together like happens now.

Ah I see. I was thinking that the solution for the Base testing setting would be making a different kind of test set, or as you say, maybe DefaultTestSet can have an option.

MichaelHatherly · 2016-07-02T19:57:45Z

docs are breaking

@kshyatt I'll try sort out the docs tests later tonight or tomorrow if possible. They probably need a bit of reorganisation since there's several modules in there that definitely won't like being wrapped in @testset.

kshyatt · 2016-07-02T20:37:02Z

@MichaelHatherly I don't think you should have to change anything. People should be able to write tests around custom modules! The problem is deserializing the module in the test file as it comes in from the remote worker. I need to figure out how to import the test module onto the master worker.

Since the docs and enums tests don't take very long, perhaps the best choice is just to run them on node1 with compile and parallel?

MichaelHatherly · 2016-07-02T21:03:08Z

OK, cool. That sounds much simpler.

kshyatt · 2016-07-02T21:36:00Z

Things are currently erroring out because we (ironically) have too many tests. I'm going to set it to get the test counts before it sends the message, and only send the full TestSet in the case of a failure/error.

kshyatt · 2016-07-03T05:21:14Z

@KristofferC @StefanKarpinski pretty enough?

nalimilan · 2016-07-03T10:15:24Z

Cool! If you want to make the output as pretty as possible, you could use the box-drawing character │ for vertical lines.

tkelman · 2016-09-29T05:57:47Z

test/libgit2.jl

@@ -793,5 +793,4 @@ mktempdir() do dir
            end
        end
    =#
-    end


this will probably be a syntax error and not bisect, should be squashed into whichever commit added it

tkelman · 2016-09-29T05:58:44Z

test/runtests.jl

@@ -104,14 +133,28 @@ cd(dirname(@__FILE__)) do
    end
    println()
    Base.Test.print_test_results(o_ts,1)
+    #pretty print the information about gc and mem usage
+    name_align    = maximum(map(x -> length(x[1]), results))


note for later, should also consider the length of the "Test:" header line here

tkelman · 2016-09-29T06:00:09Z

test/dates/io.jl

@@ -366,3 +366,5 @@ end
 @test Dates.Date("Apr 01 2014", "uuu dd yyyy") == Dates.Date(2014,4,1)
 @test_throws ArgumentError Dates.Date("Apr 01 xx 2014", "uuu dd zz yyyy")
 @test_throws ArgumentError Dates.Date("Apr 01 xx 2014", "uuu dd    yyyy")
+
+end


this was probably needed earlier to pass at intermediate commits

tkelman · 2016-09-29T06:00:41Z

test/libgit2.jl

@@ -427,6 +427,7 @@ mktempdir() do dir
            finally
                finalize(repo)
            end
+        end


probably needs to be added earlier

Make sure error-throwing works for top-level tests.

tkelman · 2016-09-29T06:04:55Z

test/arrayops.jl

@@ -1788,6 +1788,8 @@ for op in (:.+, :.*, :.÷, :.%, :.<<, :.>>, :.-, :./, :.\, :.//, :.^)
    @eval @test typeof($(op)(A,A)) == Matrix{Foo}
 end

+end


this was mistakenly deleted in an earlier commit, in order for intermediate commits to pass the re-addition should be squashed into the same commit that deleted it

tkelman · 2016-09-29T06:10:58Z

test/libgit2.jl

@@ -794,6 +799,5 @@ mktempdir() do dir
            end
        end
    =#
+    end


I see what happened. You want to end the now-commented-out @testset "SSH" begin also inside the block comment

StefanKarpinski · 2016-09-29T21:08:47Z

I propose that we just squash and merge this now that it's passing CI. I don't think it's worth the additional pain and suffering just to get a series of commits out of this that realistically no one is ever going to care about.

[ci skip]

tkelman · 2016-09-29T22:57:04Z

OK.

Follow-up for later PR: printing of @test true or @test 1==1 should be adjusted to not bother showing Expression: nothing

tkelman · 2016-09-29T23:00:22Z

test/test.jl

+    catch ex
+        #redirect_stdout(OLD_STDOUT)
+        #redirect_stderr(OLD_STDERR)
+        #@show ex


leftover debugging comments? the following tests need to test ex ~~, not ts~~

edit: so this should output ex in the catch too

kshyatt · 2016-09-30T04:45:37Z

The pretty version got merged. Let this PR be a graveyard of intermediate screencaps.

edit by tkelman: that was #18738

tkelman · 2016-12-15T20:37:24Z

I presume returning correct data for the *_exec tests will be done in a different PR?

@amitmurthy did that ever get fixed?

amitmurthy · 2016-12-16T02:43:27Z

Looking at the code, not yet.

tkelman · 2016-12-16T02:45:24Z

Should/can we open an issue to track that?

amitmurthy · 2016-12-16T02:49:27Z

Sure. I'll do it in a bit.

amitmurthy · 2016-12-16T03:45:57Z

Ref: #19620

kshyatt added the test This change adds or pertains to unit tests label Jun 28, 2016

kshyatt force-pushed the ksh/testset branch from 176ed0e to 912d83f Compare June 28, 2016 17:28

tkelman mentioned this pull request Jun 29, 2016

Fix some arith dates tests #17171

Closed

kshyatt force-pushed the ksh/testset branch from 2fc617b to 06f1b3c Compare July 2, 2016 17:11

IainNZ reviewed Jul 2, 2016
View reviewed changes

kshyatt added testsystem The unit testing framework and Test stdlib and removed test This change adds or pertains to unit tests labels Jul 3, 2016

kshyatt force-pushed the ksh/testset branch from 0f9a986 to 8a17ddc Compare July 3, 2016 08:15

kshyatt added 2 commits September 28, 2016 22:16

Wrapped more dates tests in modules to stop namespace pollution

41bba34

Cleanup of infrastructure files

22fd4ae

kshyatt force-pushed the ksh/testset branch from ede8440 to ab2b48e Compare September 29, 2016 05:38

tkelman reviewed Sep 29, 2016

View reviewed changes

test/libgit2.jl

@@ -427,6 +427,7 @@ mktempdir() do dir

finally

finalize(repo)

end

end

Copy link

Contributor

tkelman Sep 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably needs to be added earlier

kshyatt added 2 commits September 28, 2016 23:02

Add test_exec.jl to test FallbackTestset

c08ca0e

Make sure error-throwing works for top-level tests.

Fix spacing in test/math

27262ac

kshyatt force-pushed the ksh/testset branch from ab2b48e to 27262ac Compare September 29, 2016 06:02

tkelman reviewed Sep 29, 2016

View reviewed changes

add commented-out end to match testset SSH

f8413b7

[ci skip]

tkelman reviewed Sep 29, 2016

View reviewed changes

tkelman approved these changes Sep 29, 2016

View reviewed changes

kshyatt closed this Sep 30, 2016

kshyatt deleted the ksh/testset branch September 30, 2016 04:45

This was referenced Sep 30, 2016

Test printing refinement #18739

Closed

Refactoring base's runtests harness for packages to use too #18740

Closed

Implement rounding in length(::StepRange) #18744

Closed

vtjnash mentioned this pull request Oct 26, 2016

need vagrant Keno/anubis.juliacomputing.io#4

Closed

KristofferC mentioned this pull request Jan 2, 2019

testsets argument to Pkg.test() JuliaLang/Pkg.jl#981

Closed

@@ @@ -793,5 +793,4 @@ mktempdir() do dir @@
                           end
                       end
                   =#
-                  end

@@ @@ -794,6 +799,5 @@ mktempdir() do dir @@
                           end
                       end
                   =#
+                  end

[WIP] Help wanted: switch Base.Test to use testsets everywhere #17165

[WIP] Help wanted: switch Base.Test to use testsets everywhere #17165

Conversation

kshyatt commented Jun 28, 2016 • edited Loading

StefanKarpinski commented Jun 28, 2016

KristofferC commented Jun 28, 2016

kshyatt commented Jun 28, 2016

StefanKarpinski commented Jun 28, 2016

quinnj commented Jun 28, 2016

kshyatt commented Jun 28, 2016 • edited Loading

ranjanan commented Jun 28, 2016 • edited Loading

kshyatt commented Jun 28, 2016

tkelman commented Jun 28, 2016

kshyatt commented Jun 28, 2016

ranjanan commented Jun 28, 2016

kshyatt commented Jun 28, 2016

tkelman commented Jun 28, 2016

StefanKarpinski commented Jun 28, 2016

kshyatt commented Jul 2, 2016

kshyatt commented Jul 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelHatherly commented Jul 2, 2016

kshyatt commented Jul 2, 2016 • edited Loading

MichaelHatherly commented Jul 2, 2016

kshyatt commented Jul 2, 2016

kshyatt commented Jul 3, 2016

nalimilan commented Jul 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

StefanKarpinski commented Sep 29, 2016

tkelman commented Sep 29, 2016

tkelman Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

kshyatt commented Sep 30, 2016 • edited by tkelman Loading

tkelman commented Dec 15, 2016

amitmurthy commented Dec 16, 2016

tkelman commented Dec 16, 2016

amitmurthy commented Dec 16, 2016

amitmurthy commented Dec 16, 2016

kshyatt commented Jun 28, 2016 •

edited

Loading

kshyatt commented Jun 28, 2016 •

edited

Loading

ranjanan commented Jun 28, 2016 •

edited

Loading

kshyatt commented Jul 2, 2016 •

edited

Loading

tkelman Sep 29, 2016 •

edited

Loading

tkelman Sep 29, 2016 •

edited

Loading

kshyatt commented Sep 30, 2016 •

edited by tkelman

Loading