Follow useful @testset behavior

[aplavin]

Currently, @testitem behaves quite different compared to regular @testset. It would be better to follow the latter as possible, at least in some useful ways:

1. Early stopping. One can do return in @testset, and the testset stops executing. Meanwhile, return seems to be a no-op in @testitem, quite surprisingly.
2. Local scope. @testset run in local scope, while @testitem seems to have global scope. This difference manifests itself, for example, with warning for innocuous code like

o = 1
...
for i in 1:5
    o = i
    ...
end

# warns:
# Warning: Assignment to `o` in soft scope is ambiguous because a global variable by the same name exists: `o` will be treated as a new local.

I think both can be solved by wrapping code generated by @testitem into let ... end blocks.

[nickrobinson251]

In ReTestItems.jl, we think of @testitem more like a module than a testset (so 1 applies, but we've found it quite an intuitive change from testset). For 2, we "fix" that by running testitems with REPL-like "softscope".

[NHDaly]

To confirm: you aren't seeing the warnings you describe inside of a testitem, correct? We fix that via the change that Nick described.

For issue 1, yes, this is a notable departure from testsets, but it's an important one.

A testitem must be indepent, and completely isolated, so that it can be run in isolation from the VSCode interface, or in parallel, as part of runtests. For that reason, you must be able to define things like structs, functions, and modules, inside the testitem, which you cannot do in a testset.

^ This is the fundamental motivation for this difference. We try to highlight this difference in the docstrings. Please let us know if there is more that could be improved there. For example, maybe we can make this motivation more clear. Thanks!

[aplavin]

To confirm: you aren't seeing the warnings you describe inside of a testitem, correct? We fix that via the change that Nick described.

I see exactly this warning with testitems, but not testset - that's why I report it here.

Create runtests.jl with this content:

using TestItems
using TestItemRunner
@run_package_tests

@testitem "x" begin
    o = 1
    for i in 1:5
        o = i
    end
end

and run it either through ]test or through vs code test runner, and get the warning:

┌ Warning: Assignment to `o` in soft scope is ambiguous because a global variable by the same name exists: `o` will be treated as a new local. Disambiguate by using `local o` to suppress this warning or `global o` to assign to the existing global variable.

[aplavin]

For issue 1, yes, this is a notable departure from testsets, but it's an important one.

A testitem must be indepent, and completely isolated, so that it can be run in isolation from the VSCode interface, or in parallel, as part of runtests. For that reason, you must be able to define things like structs, functions, and modules, inside the testitem, which you cannot do in a testset.

Yes, I understand this part, but how is it related to my point 1?
I'm talking about early stopping that is a useful feature to have in a testset/testitem. In testset, one can do return and the test set stops executing. But neither this, nor anything else I tried, doesn't stop @testitem execution.

[davidanthoff]

At the end of the day return is only a valid statement in a function, and @testitems aren't functions, but global code in a module. So one just has to use different control flow constructs if one wants to exit early, e.g. if blocks ets.

[aplavin]

At the end of the day return is only a valid statement in a function

??
return is valid in many contexts – functions, plain REPL, testsets, surely others as well.
And in testsets, return is particularly convenient for early stopping.

[davidanthoff]

It is valid, but it doesn't do anything outside of a function. The reason it works in testsets is that under the hood that macro puts everything into a function. Test item made a different choice, namely to put everything into a module. That has many benefits: much better isolation, ability to define types etc. But it also means return won't do anything in a @testitem.

I don't want to modify the semantics of return here, right now we have a simple mental model and I want to keep it that way.

[aplavin]

It is valid, but it doesn't do anything outside of a function.

Plain REPL:

julia> for i in 1:10
       @show i
       return
       end
i = 1

Surely return does something here :)

[davidanthoff]

Ha, I have to admit I'm surprised, didn't expect that :) But the principle for testitems I think is still valid: one should think of them as modules, and I'm fairly certain return really doesn't do anything inside a module.

[stevengj]

The reason it works in testsets is that under the hood that macro puts everything into a function

@testset just wraps a let block around the code, which creates a new scope. You could still do this inside a module if you want. (Though then you can't use top-level statements like import.)

Alternatively, your macro could call REPL.softscope on the AST to give it function/REPL/@testset-like "soft" scoping behavior.

Of course, the caller could presumably also mimic @testset with

@testitem "foo" let
   # ...
end

though it might be friendlier to document this.

[kenahoo]

I just wanted to point out that the "warning" above about soft scopes isn't just nuisance chatter, it actually changes the behavior of the code relative to what would happen if the code were executed in other Julia contexts, up to & including producing a compilation error. I pointed this out in a thread here: https://discourse.julialang.org/t/soft-scopes-globals-in-testitem-s/119338

The TLDR of it is that the following test will succeed in pkg> test, but fail with LoadError: UndefVarError: `A` not defined when running in VS Code's test pane:

@testitem "foo" begin
    A = 0
    for i in 1:10
        A += i
    end
    @test A == 55
end

I think that's a big enough deal that it should be considered a bug.

[kenahoo]

@nickrobinson251 I noticed your comment:

For 2, we "fix" that by running testitems with REPL-like "softscope".

Do you mean it should already be using a softscope that should be fixing this issue? That's not what I'm seeing, is it only in a pre-release version?

EDIT: I just realized you were talking about ReTestItems.jl (as you said) and not TestItemRunner, so now I understand I shouldn't be seeing this behavior in the latter.

[davidanthoff]

@testitems have always used the hard scoping rules. I didn't think much about that at all when I created it, but at the end of the day this is probably a case where one can come up with good arguments for either choice. At this point changing it, though, would break backwards compatibility, so I don't really see that as an option.

@kenahoo the test item framework is distinct from the Julia base testing framework. Test items handle a whole lot of things differently from the base test framework, and I don't see why we should consider those different choices bugs, it's just different design decisions.

[kenahoo]

this is probably a case where one can come up with good arguments for either choice

Are there indeed good arguments why a soft scope should not be used? I certainly find the above counterintuitive behavior an argument against it, I haven't been able to think of arguments for it.

For backward compatibility considerations - are there any cases where changing to soft scope wouldn't just produce the behavior that people have already inserted workarounds to obtain (e.g. 'let' blocks or avoiding 'for' loops)? At the very least there would have been a warning a user would have to have seen and understood and accommodated somehow.