hook up stdlib to the standard test running system #24701

KristofferC · 2017-11-22T09:16:54Z

This makes it so the standard library tests run together with the other tests in the parallel test runner.

You can now also chose what stdlib test you want to run with e.g. make test-Profile. All stdlib tests can still be run with make test-stdlib but they will now run in parallel and have the nice output

Test (Worker)      | Time (s) | GC (s) | GC % | Alloc (MB) | RSS (MB)
Base64 (8)         |    1.67  |  0.02  |  1.1 | 23.41      | 161.97
CRC32c (7)         |    2.25  |  0.02  |  0.8 | 28.36      | 163.09
Profile (3)        |    6.37  |  0.03  |  0.5 | 106.00     | 190.81
Mmap (4)           |    7.05  |  1.60  | 22.7 | 85.51      | 181.50

Fixes #24191.

rfourquet

Looks good thanks! Only the makefile I didn't review.

rfourquet · 2017-11-22T09:35:19Z

test/choosetests.jl

+        dir = readdir(stdlib_dir)
+        test_file = joinpath(stdlib_dir, stdlib, "test", "runtests")
+        if isfile(test_file * ".jl")
+            unshift!(tests, test_file)


Not a big problem, but it seems that this algorithms will reverse the order of the stdlib tests (this cries for a replace! standard algorithm a la #22324 ;-) )

rfourquet · 2017-11-22T09:37:40Z

test/choosetests.jl

@@ -176,6 +178,33 @@ function choosetests(choices = [])
        filter!(x -> !(x in net_required_for), tests)
    end

+    if "stdlib" in skip_tests
+        filter!(x -> x != "stdlib", tests)


Why not skip also all test names in stdlibs, like other test groups do?

rfourquet · 2017-11-22T09:44:38Z

test/runtests.jl

@@ -13,15 +13,29 @@ else
    typemax(Csize_t)
 end

+
+test_names = copy(tests)


It seems it would be cleaner to maintain only one array tests instead of two in parallel, and when the name is needed, call a function test_name on it which implements the transormation below. Or would it make things more complicated somewhere? (or the other way around, maintaining only test_names and have a function test_path or something, cf. my next comment below).

Or maybe an array of pairs name => path or a Dict of that.

rfourquet · 2017-11-22T09:48:57Z

test/choosetests.jl

+    if "stdlib" in skip_tests
+        filter!(x -> x != "stdlib", tests)
+    elseif "stdlib" in tests
+        filter!(x -> x != "stdlib", tests)


similarly here, filter out individual stdlib tests?

rfourquet · 2017-11-22T09:56:32Z

test/runtests.jl

+    if contains(test, "stdlib") && endswith(test, "runtests")
+        test_names[i] = split(test, "/")[end-2]
+    end
+end


It's a bit funny to transorm test names to "runtest.jl" files (in "choosetest.jl"), and then back here to a name. Maybe infering the test file name could be implemented only here in this file?

KristofferC · 2017-11-22T10:36:37Z

Thanks for your comments @rfourquet. I felt the same way when I implemented it that the two array style was pretty ugly. I think I have a better design coming up.

KristofferC · 2017-11-22T10:44:42Z

New version up, @rfourquet

rfourquet · 2017-11-22T10:53:07Z

test/runtests.jl

                    catch e
                        resp = [e]
                    end
                    push!(results, (test, resp))
                    if resp[1] isa Exception
                        if exit_on_error
-                            skipped = length(tests)
-                            empty!(tests)
+                            skipped = length(test_data)


Forgot to change back test_data to tests?

rfourquet · 2017-11-22T10:59:22Z

test/runtests.jl

+    if test in STDLIBS
+        test_file = joinpath(STDLIB_DIR, test, "test", "runtests")
+        if !isfile(test_file * ".jl")
+            error("Standard library $test did not provide a `test/runtests.jl` file")


This may be beyond the scope of this PR, but given #24628, there may or may not be a nicer way to report the error? (I didn't investigate more this question, and definitely non-blocking).

rfourquet · 2017-11-22T11:02:36Z

test/testdefs.jl

@@ -2,7 +2,7 @@

 using Test

-function runtests(name, isolate=true; seed=nothing)
+function runtests(name, path, isolate=true; seed=nothing)


As this file is include'd, doesn't this function know about test_path ? (to avoid passing the path argument here)

I think I prefer to keep it explicit like this to potentially ease the use of this file in other contexts.

Ok no problem. But maybe then as in most of the cases the name is equal to the path, there could be a default, or even better a keyword path=name. But also I find it tricky that the path is still not the real path, as ".jl" has to be added. I guess this logic comes from the use of name before, but I see no reason now that path should not contain the .jl extension. Then the default value for path would be name * ".jl".

KristofferC · 2017-11-22T13:47:45Z

	From worker 1:		From worker 1:		From worker 1:	Distributed: Error During Test at nothing:1
  Test threw an exception of type ErrorException
  Expression: Distributed
  Standard library Distributed did not provide a `test/runtests.jl` file

Anyone know why Distributed gets put with the other stdlib packages in the share dir? Do I need an explicit stdlib list?

iblislin · 2017-11-22T16:13:21Z

hmm I think it's the side effect from #24443 on my worker.

Seems there are some leftovers not control by git...

[venv] % pwd
/usr/home/julia/julia-fbsd-buildbot/worker/11rel-amd64/build/stdlib
[venv] % find Distributed/
Distributed/
Distributed/src
Distributed/src/remotecall.jl.86100.cov
Distributed/src/remotecall.jl.86101.cov
Distributed/src/remotecall.jl.mem

I can add some rm instructions to my worker
Any better way to clean up them?

vtjnash · 2017-11-22T16:17:03Z

Might make sense to run git clean -fd (Without -x, so it leaves behind ignored files)?

KristofferC · 2017-11-22T16:17:45Z

I was so confused because #24443 wasn't merged yet. Your explanation makes a lot of sense. Some cleanup seems to be needed but I am not sure what the best way to do it is.

iblislin · 2017-11-22T16:28:27Z

Might make sense to run git clean -fd (Without -x, so it leaves behind ignored files)?

well, I just found that *.cov and *.mem is listed in .gitignore
so... git clean -fd will not work.

iblislin · 2017-11-22T16:29:42Z

I'm going to rm stdlib manually, since stdlib is changing quite rapidly.

iblislin · 2017-11-22T16:32:22Z

config changed... let's wait for the queue.

KristofferC · 2017-11-22T16:33:15Z

The problem are not the cov fiels but the Delimited directory.

Also, these files exist in julia/usr/share/site/ not stdlib.

iblislin · 2017-11-22T16:41:40Z

The problem are not the cov fiels but the Delimited directory.

You mean the Distrbuted dir, right?

Actually, I configured buildbot with git clean -d -f -f after checkout. It will keep ignored file, so our deps will be cached. But in the case of Distrubuted, it generated some files which ignored by git, so the dir isn't clean and cannot be removed. So I consider the cov file is the root cause.

KristofferC · 2017-11-22T16:43:52Z

Oh, I see (and yes I meant Distributed).

iblislin · 2017-11-22T16:45:38Z

Also, these files exist in julia/usr/share/site/ not stdlib.

Okay, I should do rm usr/share/site/ for each build?

KristofferC · 2017-11-22T16:46:36Z

I would think so yes, but I am not really knowledgeable how the files are copied when building...

iblislin · 2017-11-22T16:49:33Z

Oh, I saw that usr/share/julia/site/v0.7 just a soft link, so rm stdlib should be enough.

iblislin · 2017-11-22T16:52:57Z

/me going to 💤 , 🙏 it works as expected :p

the build queue is quite long now(https://julia.iblis.cnmc.tw/#/builders/1)...

KristofferC · 2017-11-22T19:50:35Z

@iblis17 Looks like what you did worked great. This should be good to go from my pov.

* hook up stdlib to the standard test running system * fixups from review

hook up stdlib to the standard test running system

660848c

KristofferC added the testsystem The unit testing framework and Test stdlib label Nov 22, 2017

rfourquet approved these changes Nov 22, 2017

View reviewed changes

fixups from review

8f197bd

KristofferC force-pushed the kc/run_stdlib_parallel branch from b2e4780 to 8f197bd Compare November 22, 2017 10:59

rfourquet reviewed Nov 22, 2017

View reviewed changes

KristofferC closed this Nov 22, 2017

KristofferC reopened this Nov 22, 2017

kshyatt added the stdlib Julia's standard library label Nov 22, 2017

KristofferC merged commit fb2ba43 into master Nov 23, 2017

KristofferC added a commit that referenced this pull request Nov 23, 2017

hook up stdlib to the standard test running system (#24701)

750b821

* hook up stdlib to the standard test running system * fixups from review

StefanKarpinski deleted the kc/run_stdlib_parallel branch November 23, 2017 12:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hook up stdlib to the standard test running system #24701

hook up stdlib to the standard test running system #24701

KristofferC commented Nov 22, 2017 •

edited

Loading

rfourquet left a comment

rfourquet Nov 22, 2017

rfourquet Nov 22, 2017

rfourquet Nov 22, 2017

rfourquet Nov 22, 2017

rfourquet Nov 22, 2017

rfourquet Nov 22, 2017

KristofferC commented Nov 22, 2017

KristofferC commented Nov 22, 2017

rfourquet Nov 22, 2017

rfourquet Nov 22, 2017

rfourquet Nov 22, 2017

KristofferC Nov 22, 2017

rfourquet Nov 22, 2017

KristofferC commented Nov 22, 2017 •

edited

Loading

iblislin commented Nov 22, 2017 •

edited

Loading

vtjnash commented Nov 22, 2017

KristofferC commented Nov 22, 2017

iblislin commented Nov 22, 2017

iblislin commented Nov 22, 2017

iblislin commented Nov 22, 2017

KristofferC commented Nov 22, 2017 •

edited

Loading

iblislin commented Nov 22, 2017

KristofferC commented Nov 22, 2017

iblislin commented Nov 22, 2017

KristofferC commented Nov 22, 2017

iblislin commented Nov 22, 2017

iblislin commented Nov 22, 2017

KristofferC commented Nov 22, 2017

hook up stdlib to the standard test running system #24701

hook up stdlib to the standard test running system #24701

Conversation

KristofferC commented Nov 22, 2017 • edited Loading

rfourquet left a 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

KristofferC commented Nov 22, 2017

KristofferC commented Nov 22, 2017

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

KristofferC commented Nov 22, 2017 • edited Loading

iblislin commented Nov 22, 2017 • edited Loading

vtjnash commented Nov 22, 2017

KristofferC commented Nov 22, 2017

iblislin commented Nov 22, 2017

iblislin commented Nov 22, 2017

iblislin commented Nov 22, 2017

KristofferC commented Nov 22, 2017 • edited Loading

iblislin commented Nov 22, 2017

KristofferC commented Nov 22, 2017

iblislin commented Nov 22, 2017

KristofferC commented Nov 22, 2017

iblislin commented Nov 22, 2017

iblislin commented Nov 22, 2017

KristofferC commented Nov 22, 2017

KristofferC commented Nov 22, 2017 •

edited

Loading

KristofferC commented Nov 22, 2017 •

edited

Loading

iblislin commented Nov 22, 2017 •

edited

Loading

KristofferC commented Nov 22, 2017 •

edited

Loading