Test Project.toml structure and suppressor

[lkdvos]

As discussed with @mtfishman, we switch to the other convention of specifying test dependencies, using a test/Project.toml file instead of the [extras] section in the main project file.

Simultaneously, I fixed an issue where the examples testsets were being suppressed.

As a remark, at this point it might become cleaner to define our own @safetestset which supports interpolation, and @safeexampleset which suppresses the output. TestExtras.jl might be a convenient place to put this.

Closes #35
Closes #36

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 6.02%. Comparing base (07abef5) to head (3391dd4).
Report is 2 commits behind head on main.

Additional details and impacted files

@@          Coverage Diff          @@
##            main     #39   +/-   ##
=====================================
  Coverage   6.02%   6.02%           
=====================================
  Files          1       1           
  Lines         83      83           
=====================================
  Hits           5       5           
  Misses        78      78           

Flag	Coverage Δ
docs	0.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mtfishman]

As a remark, at this point it might become cleaner to define our own @safetestset which supports interpolation, and @safeexampleset which suppresses the output. TestExtras.jl might be a convenient place to put this.

Agreed, I stopped using @safetestset since I was finding too many issues with it. For now can we just use a combination of @testset and @eval module $(gensym())? @safetestset doesn't seem worth using if there are so many hoops to jump through.

[lkdvos]

For what it's worth, the eval hoops would have to remain there as well, you would still have to interpolate a string into the module or write out the suppressor dependency, since the problem is really that the module construction does what it's supposed to do: it creates an isolated environment. In that sense, I kind of like the @safetestset name, since it tells a bit more what is going on than the module $(gensym()), so I think my preference would be to just merge this as-is for now, and try and add this to TestExtras.jl later?

[mtfishman]

For what it's worth, the eval hoops would have to remain there as well, you would still have to interpolate a string into the module or write out the suppressor dependency, since the problem is really that the module construction does what it's supposed to do: it creates an isolated environment. In that sense, I kind of like the @safetestset name, since it tells a bit more what is going on than the module $(gensym()), so I think my preference would be to just merge this as-is for now, and try and add this to TestExtras.jl later?

Sure, that's fine. Though can we simplify the current PR by putting using Suppressor: @suppress inside @safetestset rather than using GlobalRef? I think that is a better style anyway since it is more explicit, and as you say it is what you generally have to do anyway inside a new module.

So ideally, we should be able to write it as something like:

    @safetestset "$file" for file in files
      using Suppressor: @suppress
      @suppress include(file)
    end

with a new TextExtras.@safetestset macro, right?

EDIT: Which could then be defined for convenience as:

    @safeexampleset "$file" for file in files
      include(file)
    end

as you suggested in the first post.

[lkdvos]

Let me doublecheck, I was playing around with that this morning and somehow couldn't get that to work, but it might be because I was doing something else wrong.

[lkdvos]

Yeah, that doesn't actually work. The problem is that safetestset expands to:

julia> @eval module $(gensym())
       using Test, SafeTestsets
       @testset "test.jl" begin
       using Suppressor: @suppress
       @suppress include("path/test.jl")
       end
       end
ERROR: LoadError: UndefVarError: `@suppress` not defined in `Main.var"##232"`
Suggestion: check for spelling errors or missing imports.

The problem is the placement of the using Suppressor statement, which is supposed to be inside of the module but outside of the testset, and there is no way of inserting code there with Safetestsets.jl. In that regards, I would say that it is a bit of a strange design choice, as I would guess that the following is more robust:

@testset "$name" begin
module $(gensym())
include(...)
end
end

(I didn't check if @testset allows that though.)

Linking the following:
Jutho/TestExtras.jl#8

[mtfishman]

Gotcha, thanks for checking. If @testset doesn't allow that, TestExtras.@safetestset could have a "setup" argument for code that should be executed before @testset, i.e.:

@safetestset setup=(using Suppressors: @suppress) begin
  @suppress include("path/test.jl")
end

[lkdvos]

julia> @testset begin
       module MyTest
       using Test
       @test 1 == 1
       @test 1==2
       end
       end
ERROR: syntax: "module" expression not at top level
Stacktrace:
 [1] top-level scope
   @ REPL[4]:1

This definitely doesn't work, so that kind of scratches this attempt.
The setup code is definitely quite elegant, but maybe we don't really want to promote putting dependencies in a setup block, since this is again injecting code into the test: running the testfile by itself would be different than running the file from the runtests.jl, since you have the additional setup code running there.
As an alternative option, I think the insertion of @suppress with a globalref is actually quite nice, since it doesn't pollute the namespace, it's just very ugly to type, so maybe we could just have an @safeexampleset macro which automatically takes care of that?

[mtfishman]

As an alternative option, I think the insertion of @suppress with a globalref is actually quite nice, since it doesn't pollute the namespace, it's just very ugly to type, so maybe we could just have an @safeexampleset macro which automatically takes care of that?

That seems fine. The part that made me uncomfortable about that is that it is hardcoded to @suppress, i.e. it isn't an extensible interface. But I can't think of other cases where you need something like this, since mostly you should be putting things in the test files themselves and making the test files self-contained, i.e. as you say we probably shouldn't encourage people to add too much setup code anyway. So let's keep this PR as-is and plan to simplify it with @safeexampleset.