Requires.jl icon indicating copy to clipboard operation
Requires.jl copied to clipboard

Doesn't compose well with testsets

Open staticfloat opened this issue 4 years ago • 3 comments

I have found a few usages in the wild of @require enabling test sets dynamically, but it has some surprising interactions in that a failing test within the nested @require-@testset gets printed, but does not impact Julia's return value. Example:

# test.jl
using Test, Requires                                                                                                                                               

@require Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40" begin
    @testset "Linux-specific Tests" begin
        @test false
    end
end
$ julia test.jl && echo yes
Linux-specific Tests: Test Failed at /home/sabae/src/test.jl:5
  Expression: false
Stacktrace:
 [1] macro expansion
   @ ~/src/test.jl:5 [inlined]
 [2] macro expansion
   @ /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/Test/src/Test.jl:1151 [inlined]
 [3] top-level scope
   @ ~/src/test.jl:5
Test Summary:        | Fail  Total
Linux-specific Tests |    1      1
┌ Warning: Error requiring `Test` from `Main`
│   exception = Some tests did not pass: 0 passed, 1 failed, 0 errored, 0 broken.
└ @ Requires ~/.julia/packages/Requires/7Ncym/src/require.jl:49
yes

It looks to me like a failing test set might get caught by some generic error handling routine.

staticfloat avatar Apr 05 '21 20:04 staticfloat

https://github.com/JuliaPackaging/Requires.jl/blob/7ff79f692d43658c285a80aa10dc29fa29921049/src/require.jl#L45-L51 I guess. I don't really understand why you wouldn't just error there, something is cleary broken.

KristofferC avatar Apr 06 '21 15:04 KristofferC

Seems to me that we should rethrow(exc)?

staticfloat avatar Apr 06 '21 20:04 staticfloat

Imo yes. But maybe there is some good reason. I guess it can be argued that if the @requires part is an optional addition of functionality, failing to load that shouldn't be a hard error. But it is unclear what state the system is in after that part errors so in my opinion error is probably the right thing to do.

KristofferC avatar Apr 07 '21 09:04 KristofferC