[WIP/RFC] Rewrite testing infrastructure to live in Base #19567

kshyatt · 2016-12-12T20:55:05Z

Move testdefs.jl to base. Modify runtests and write some
utility functions to let base Julia and packages use this function.

Now, a package can provide its own runtests.jl, or it can simply provide a choosetests.jl analogous to the one in Julia's test/. I wrote an example for Combinatorics.jl that looks like:

function choosetests(choices = [])
    testnames = ["numbers", "factorials", "combinations", "permutations", "partitions", "youngdiagrams"]
    tests = []
    skip_tests = []

    for (i, t) in enumerate(choices)
        if t == "--skip"
            skip_tests = choices[i + 1:end]
            break
        else
            push!(tests, t)
        end
    end

    if tests == ["all"] || isempty(tests)
        tests = testnames
    end

    node1_tests = []
    bigmemtests = []
    tests, node1_tests, bigmemtests, false
end

Just like the main Julia tests, this will allow package writers to specify if some tests use lots of memory or need to run on the first worker for other reasons. Otherwise, they can run tests in parallel using Julia's infrastructure rather than rolling their own. They'll also get to print the same statistics, for instance:

To do:

Document how to make this work for packages in the packages manual
Clean up testdefs (@yuyichao, your advice might be helpful?)
Fix Travis
Test against a variety of packages

Move `testdefs.jl` to base. Modify `runtests` and write some utility functions to let base Julia *and* packages use this function.

yuyichao · 2016-12-12T21:10:09Z

test/Makefile

@@ -18,7 +18,7 @@ endif
 $(TESTS):
 	@cd $(SRCDIR) && \
 	$(ULIMIT_TEST) \
-	$(call PRINT_JULIA, $(call spawn,$(JULIA_EXECUTABLE)) --check-bounds=yes --startup-file=no ./runtests.jl $@)


Can we make sure we still have this file? It's very useful to run the tests without going through makefile in order to specify different options.

Sure, I can put it back in.

That will also un-break Travis and AV, probably 😓

Done! runtests.jl is back as a stub

ararslan · 2016-12-12T21:14:45Z

base/test.jl

+        if net_on
+            n = min(Sys.CPU_CORES, length(tests))
+            n > 1 && addprocs(n; exename=test_exename, exeflags=test_exeflags)
+            BLAS.set_num_threads(1)


Does this have to be set back to the default after performing the task that requires a single thread?

tkelman · 2016-12-12T21:30:58Z

base/interactiveutil.jl

-Run the Julia unit tests listed in `tests`, which can be either a string or an array of
-strings, using `numcores` processors. (not exported)
-"""
-function runtests(tests = ["all"], numcores = ceil(Int, Sys.CPU_CORES / 2))


this should be deprecated

Done (hopefully!)

tkelman · 2016-12-12T21:35:32Z

base/sysimg.jl

@@ -294,6 +294,7 @@ include("deepcopy.jl")
 include("interactiveutil.jl")
 include("replutil.jl")
 include("test.jl")
+include("testdefs.jl")


this should be in Base.Test, not Base

kshyatt · 2016-12-12T22:30:53Z

Should I add a kwarg to my base/test.jl runtests to specify the number of cores, like the function in interactiveutil.jl had?

aviks · 2016-12-13T09:59:32Z

Very cool. Will packages that support 0.5, or even 0.4, be able to use this at all. Even if not, maybe useful make the migration path clear in the documentation.

kshyatt · 2016-12-15T00:48:39Z

Any thoughts on why I'm hitting mem limit? Do I need to set the max rss in test/runtests as well?

yuyichao · 2016-12-15T00:56:18Z

base/test.jl

+    end
+
+    if haskey(ENV, "JULIA_TEST_EXEFLAGS")
+        const test_exeflags = `$(Base.shell_split(ENV["JULIA_TEST_EXEFLAGS"]))`


Sounds good

yuyichao · 2016-12-15T01:00:16Z

base/test.jl

+end
+
+function runtests(names=["all"]; test_dir=joinpath(JULIA_HOME, Base.DATAROOTDIR, "julia/test/"), numcores::Int=ceil(Int, Sys.CPU_CORES / 2))
+    include(joinpath(test_dir, "choosetests.jl"))


I believe these includes should just move out of the function. This function shouldnt call include except on the test file

The includes are in the function in case you want to pass the package's test dir in, and you can have your own choosetests.jl in Pkg.dir()/test

That doesn't sound like a good API. And it also doesn't apply to testdefs.jl, which should just be part of Base.

If a package needs to customize the tests it wants to run, it should just do that in runtests.jl and pass the test list to Base.runtests().

AFAICT doing this in choosetests.jl instead of runtests.jl doesn't really make it any easier for the packages. The most useful part of the base choosetests.jl is (IMHO) the --skip option and the all "meta"-test and this currently doesn't make it possible for the package to use it. It might be useful to provide a few helper functions for cmdline parsing but I honestly don't think the one we have in base is a particularly good one and packages are better off using sth like ArgParse.jl for that.

Even if a choosetests type of interface is wanted, it should be implemented as a callback rather than includeing a file.

kshyatt · 2016-12-15T19:39:06Z

Thanks for the detailed feedback, @yuyichao. The power of [RFC]! Do others (@tkelman, @StefanKarpinski) feel similarly? I'm happy to abandon this avenue if we think it won't be productive.

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

This'll take some design iteration I think. In #18740 I listed the following features as goals to make easy for packages to use the same functionality as base:

parallel execution
easily running subsets of tests (related: testsets argument to Pkg.test() #15404)
pretty-printing

points 1 and 3 shouldn't be too complicated, it's point 2 that may need some feedback from package authors about how they'd like it to work. I'm also not sure if packages writing their own choosetests.jl is the best way to go about it, but I also haven't written packages with enough tests to make good use of this kind of thing. Maybe worth pinging for feedback on discourse with a cross-link here?

KristofferC · 2016-12-24T23:06:32Z

I like this initiative. One of the thing that I don't like so much with package-testing (á la Pkg.test()) is that tests are just normal julia code that gets run from top to bottom with exceptions being thrown when an error is detected. In other words, there is no "Test Manager" that is one level above the tests being executed. I think this is needed for better pretty printing and error reporting.

So what I want is something along the lines where packages do not have a runtests.jl file that simply includes everything. Instead a bunch of independent test_x.jl exists in the test directory and it is the "Test Manager's" job to execute these, do pretty printing of passes and errors and exit the process with success or fail.

Am I correct that this PR would enable functionality for packages to do something like that.

ed1d1a8d · 2019-05-23T22:37:24Z

Any updates on these functionalities, particularly the ability to run tests parallel?

StefanKarpinski · 2019-05-24T18:32:32Z

No, unfortunately this effort stalled out but it's still very much wanted.

ed1d1a8d · 2019-05-26T05:14:20Z

If I'm interested in taking up this effort, where would be the place to start?

Rewrite testing infrastructure to live in Base

61c3af4

Move `testdefs.jl` to base. Modify `runtests` and write some utility functions to let base Julia *and* packages use this function.

kshyatt added needs docs Documentation for this change is required packages Package management and loading testsystem The unit testing framework and Test stdlib labels Dec 12, 2016

kshyatt requested a review from tkelman December 12, 2016 20:55

yuyichao reviewed Dec 12, 2016

View reviewed changes

ararslan reviewed Dec 12, 2016

View reviewed changes

tkelman reviewed Dec 12, 2016

View reviewed changes

kshyatt added 3 commits December 12, 2016 15:09

Fix deprecation, name conflict, restore test/runtests

a0c7f0f

Add numcores kwarg to runtests

89e8541

Trial fix to Travis

ef7ac55

kshyatt added 3 commits December 14, 2016 11:03

Restore old Travis

bb070ff

Pass through the right test directory for test/runtests

55c2c61

use Base.DATAROOTDIR

5799d1b

yuyichao requested changes Dec 15, 2016

View reviewed changes

Get rid of useless consts

ffa26ba

This was referenced Jan 20, 2017

Added ConjArray wrapper type for conjugate array views #20047

Merged

bring back testset printing #20221

Merged

kshyatt mentioned this pull request Feb 1, 2017

CI logs do not report testset stats for tests run on node 1 #20362

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP/RFC] Rewrite testing infrastructure to live in Base #19567

[WIP/RFC] Rewrite testing infrastructure to live in Base #19567

kshyatt commented Dec 12, 2016 •

edited

Loading

yuyichao Dec 12, 2016

kshyatt Dec 12, 2016

kshyatt Dec 12, 2016

kshyatt Dec 13, 2016

ararslan Dec 12, 2016

tkelman Dec 12, 2016

kshyatt Dec 13, 2016

tkelman Dec 12, 2016

kshyatt commented Dec 12, 2016

aviks commented Dec 13, 2016

kshyatt commented Dec 15, 2016

yuyichao Dec 15, 2016

kshyatt Dec 15, 2016

yuyichao Dec 15, 2016

kshyatt Dec 15, 2016

yuyichao Dec 15, 2016

yuyichao Dec 15, 2016

kshyatt commented Dec 15, 2016

tkelman commented Dec 15, 2016

KristofferC commented Dec 24, 2016

ed1d1a8d commented May 23, 2019

StefanKarpinski commented May 24, 2019

ed1d1a8d commented May 26, 2019

[WIP/RFC] Rewrite testing infrastructure to live in Base #19567

Are you sure you want to change the base?

[WIP/RFC] Rewrite testing infrastructure to live in Base #19567

Conversation

kshyatt commented Dec 12, 2016 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kshyatt commented Dec 12, 2016

aviks commented Dec 13, 2016

kshyatt commented Dec 15, 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

Choose a reason for hiding this comment

kshyatt commented Dec 15, 2016

tkelman commented Dec 15, 2016

KristofferC commented Dec 24, 2016

ed1d1a8d commented May 23, 2019

StefanKarpinski commented May 24, 2019

ed1d1a8d commented May 26, 2019

kshyatt commented Dec 12, 2016 •

edited

Loading