JuliaPackaging / Requires.jl

Lazy code loading for Julia
Other
195 stars 28 forks source link

Doesn't compose well with testsets #103

Open staticfloat opened 3 years ago

staticfloat commented 3 years ago

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.

KristofferC commented 3 years ago

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.

staticfloat commented 3 years ago

Seems to me that we should rethrow(exc)?

KristofferC commented 3 years ago

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.