Run previously failed tests first

src/ParallelTestRunner.jl

@@ -514,25 +514,34 @@ function default_njobs(; cpu_threads = Sys.CPU_THREADS, free_memory = available_
     return max(1, min(jobs, memory_jobs))
 end
 
+# Struct used in runtests to sort failed tests before successful ones
+struct TestHistoryEntry
+    duration::Float64
+    failed::Bool
+end
+# successful tests < failed tests, so when reversing the
+# sort they are also in proper descending order
+Base.isless(a::TestHistoryEntry, b::TestHistoryEntry) = a.failed == b.failed ? a.duration < b.duration : a.failed < b.failed
+
 # Historical test duration database
 function get_history_file(mod::Module)
+    # History file version. Change when modifying the history format
+    hist_ver = "v2"
     scratch_dir = @get_scratch!("durations")
-    return joinpath(scratch_dir, "v$(VERSION.major).$(VERSION.minor)", "$(nameof(mod)).jls")
+    return joinpath(scratch_dir, "v$(VERSION.major).$(VERSION.minor)", hist_ver, "$(nameof(mod)).jls")
 end
 function load_test_history(mod::Module)
     history_file = get_history_file(mod)
     if isfile(history_file)
         try
-            return deserialize(history_file)
+            return deserialize(history_file)::Tuple{Dict{String, Float64}, Set{String}}
         catch e
             @warn "Failed to load test history from $history_file" exception=e
-            return Dict{String, Float64}()
         end
-    else
-        return Dict{String, Float64}()
     end
+    return (Dict{String, Float64}(), Set{String}())
 end
-function save_test_history(mod::Module, history::Dict{String, Float64})
+function save_test_history(mod::Module, history::Tuple{Dict{String, Float64}, Set{String}})
     history_file = get_history_file(mod)
     try
         mkpath(dirname(history_file))
@@ -991,14 +1000,16 @@ function runtests(mod::Module, args::ParsedArgs;
     # determine test order
     tests = collect(keys(testsuite))
     Random.shuffle!(tests)
-    historical_durations = load_test_history(mod)
-    sort!(tests, by = x -> -get(historical_durations, x, Inf))
+    historical_durations, historical_failures = load_test_history(mod)
+    get_historical_duration(test) = TestHistoryEntry(get(historical_durations, test, Inf), test in historical_failures)
+    sort!(tests, by = x -> get_historical_duration(x), rev = true)
 
     return _runtests(
         mod, args;
         testsuite,
         tests,
         historical_durations,
+        historical_failures,
         init_code,
         init_worker_code,
         test_worker,
@@ -1019,7 +1030,8 @@ end
 function _runtests(mod::Module, args::ParsedArgs;
                    testsuite::Dict{String,Expr} = find_tests(pwd()),
                    tests::Vector{String},
-                   historical_durations::Dict{String, Float64},
+                   historical_durations::Dict{String, Float64} = Dict{String, Float64}(),
+                   historical_failures::Set{String} = Set{String}(),
                    init_code = :(),
                    init_worker_code = :(),
                    test_worker = Returns(nothing),
@@ -1521,6 +1533,9 @@ function _runtests(mod::Module, args::ParsedArgs;
                 if result isa AbstractTestRecord
                     testset = result[]::DefaultTestSet
                     historical_durations[testname] = stop - start
+                    # push to historical_failures on failure and delete on success
+                    push_or_delete! = anynonpass(testset) ? push! : delete!
+                    push_or_delete!(historical_failures, testname)
                 else
                     # If this test raised an exception that means the test runner itself had some problem,
                     # so we may have hit a segfault, deserialization errors or something similar.
@@ -1529,6 +1544,7 @@ function _runtests(mod::Module, args::ParsedArgs;
                     @assert result isa Exception
                     testset = create_testset(testname; start, stop)
                     Test.record(testset, Test.Error(:nontest_error, testname, nothing, Base.ExceptionStack(NamedTuple[(;exception = result, backtrace = Union{Ptr{Nothing}, Base.InterpreterIP}[])]), LineNumberNode(1)))
+                    push!(historical_failures, testname)
                 end
 
                 with_testset(testset) do
@@ -1560,7 +1576,7 @@ function _runtests(mod::Module, args::ParsedArgs;
             Test.TESTSET_PRINT_ENABLE[] = old_print_setting
         end
     end
-    save_test_history(mod, historical_durations)
+    save_test_history(mod, (historical_durations, historical_failures))
 
     # display the results
     println(io_ctx.stdout)

test/runtests.jl

@@ -102,7 +102,6 @@ end
         ParallelTestRunner, parse_args(["--jobs=1", "--verbose"]);
         testsuite,
         tests,
-        historical_durations=Dict{String, Float64}(),
         stdout=io,
         stderr=io,
     )
@@ -827,6 +826,31 @@ end
     @test any(contains("--color=yes"), exe.exec)
 end
 
+@testset "TestHistoryEntry" begin
+    flow = ParallelTestRunner.TestHistoryEntry(1,true)
+    fhigh = ParallelTestRunner.TestHistoryEntry(10,true)
+
+    slow = ParallelTestRunner.TestHistoryEntry(1,false)
+    shigh = ParallelTestRunner.TestHistoryEntry(10,false)
+
+    # irreflexive: lt(x, x) always yields false
+    @test !isless(flow, flow)
+    @test !isless(slow, slow)
+    @test !isless(fhigh, fhigh)
+    @test !isless(shigh, shigh)
+
+    # asymmetric: if lt(x, y) yields true then lt(y, x) yields false
+    @test isless(flow, fhigh)
+    @test !isless(fhigh, flow)
+    @test isless(shigh, flow)
+    @test !isless(flow, shigh)
+
+    # transitive: lt(x, y) && lt(y, z) implies lt(x, z)
+    @test isless(slow, shigh)
+    @test isless(shigh, flow)
+    @test isless(slow, flow)
+end
+
 # ── Integration tests ────────────────────────────────────────────────────────
 
 @testset "non-verbose mode" begin
@@ -862,7 +886,6 @@ end
             ParallelTestRunner, parse_args(["--quickfail", "--verbose", "--jobs=1"]);
             testsuite,
             tests=["fail-test", "pass-test1", "pass-test2", "pass-test3", "pass-test4", "pass-test5"],
-            historical_durations=Dict{String, Float64}(),
             stdout=io,
             stderr=io,
         )
@@ -1183,7 +1206,6 @@ end
                 testsuite,
                 tests=["fail-serial", "pass-serial1", "pass-serial2",
                        "pass-parallel1", "pass-parallel2", "pass-parallel3"],
-                historical_durations=Dict{String, Float64}(),
                 serial=["fail-serial", "pass-serial1", "pass-serial2"],
                 stdout=io,
                 stderr=io,
@@ -1218,7 +1240,6 @@ end
                 testsuite,
                 tests=["fail-parallel", "pass-parallel1", "pass-parallel2",
                        "pass-serial1", "pass-serial2"],
-                historical_durations=Dict{String, Float64}(),
                 serial=["pass-serial1", "pass-serial2"],
                 serial_position=:after,
                 stdout=io,
@@ -1253,7 +1274,6 @@ end
                 testsuite,
                 tests=["pass-parallel1", "pass-parallel2",
                        "fail-serial", "pass-serial1", "pass-serial2"],
-                historical_durations=Dict{String, Float64}(),
                 serial=["fail-serial", "pass-serial1", "pass-serial2"],
                 serial_position=:after,
                 stdout=io,
@@ -1271,6 +1291,62 @@ end
         # The serial tests after `fail-serial` are never started.
         @test !contains(str, r"pass-serial[12] .+ started at")
     end
+
+    @testset "run previously failed tests first" begin
+        # a previously-failed test must start before previously-passing tests,
+        # even if it has a much shorter historical duration than they do
+        mod = @eval(Main, module $(gensym(:failfirstserial)) end)
+
+        testsuite = Dict(
+            "long-serial-pass" => :(@test true),
+            "mid-omitted-serial-pass" => :(@test true),
+            "short-serial-fail" => :(@test true),
+            "long-pass" => :(@test true),
+            "mid-omitted-pass" => :(@test true),
+            "mid-pass" => :(@test true),
+            "short-fail" => :(@test true),
+        )
+        serial = filter(collect(keys(testsuite))) do k
+            contains(k, "serial")
+        end
+
+        ParallelTestRunner.save_test_history(mod, (Dict(
+            "long-serial-pass" => 10.0,
+            "short-serial-fail" => 1.0,
+            "long-pass" => 10.0,
+            "mid-pass" => 5.0,
+            "short-fail" => 1.0,
+        ), Set(["short-serial-fail", "short-fail"])))
+
+        io = IOBuffer()
+        runtests(
+            mod, parse_args(["--jobs=1", "--verbose"]);
+            testsuite,
+            stdout=io,
+            stderr=io,
+            serial
+        )
+
+        str = String(take!(io)); print(str)
+        @test contains(str, "SUCCESS")
+
+        # create a mapping of test names to the character offset of their start times (lower is earlier)
+        started_at = Dict(
+            name => (m = match(Regex("$(name) .+ started at"), str); @test m !== nothing; m === nothing ? typemax(Int) : m.offset)
+            for name in keys(testsuite)
+        )
+
+        # normal
+        @test started_at["short-fail"] < started_at["mid-pass"]
+        @test started_at["short-fail"] < started_at["long-pass"]
+        @test started_at["short-fail"] < started_at["mid-omitted-pass"]
+        # among the remaining (previously-passing) tests, longer ones still run first
+        @test started_at["long-pass"] < started_at["mid-pass"]
+
+        # serial
+        @test started_at["short-serial-fail"] < started_at["long-serial-pass"]
+        @test started_at["short-serial-fail"] < started_at["mid-omitted-serial-pass"]
+    end
 end
 
 # This testset should always be the last one, don't add anything after this.