Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change in behavior of UndefVarError(::Symbol) constructor breaks @test_throws in 1.11.0-beta1 #54082

Closed
DilumAluthge opened this issue Apr 14, 2024 · 8 comments · Fixed by #56231
Labels
error handling Handling of exceptions by Julia or the user testsystem The unit testing framework and Test stdlib

Comments

@DilumAluthge
Copy link
Member

DilumAluthge commented Apr 14, 2024

MWE

Here is the MWE:

using Test: @test_throws

module MyModule end

foo() = MyModule.x

@test_throws UndefVarError(:x) foo()

On Julia 1.10.2, this MWE works fine:

  | | |_| | | | (_| |  |  Version 1.10.2 (2024-03-01)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release

julia> using Test: @test_throws

julia> module MyModule end
Main.MyModule

julia> foo() = MyModule.x
foo (generic function with 1 method)

julia> @test_throws UndefVarError(:x) foo()
Test Passed
      Thrown: UndefVarError

On Julia 1.11.0-beta1, the MWE is broken:

  | | |_| | | | (_| |  |  Version 1.11.0-beta1 (2024-04-10)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release

julia> using Test: @test_throws

julia> module MyModule end
Main.MyModule

julia> foo() = MyModule.x
foo (generic function with 1 method)

julia> @test_throws UndefVarError(:x) foo()
ERROR: UndefRefError: access to undefined reference
Stacktrace:
 [1] do_test_throws(result::Test.ExecutionResult, orig_expr::Any, extype::Any)
   @ Test ~/.asdf/installs/julia/1.11.0-beta1/share/julia/stdlib/v1.11/Test/src/Test.jl:815
 [2] top-level scope
   @ REPL[4]:1
Similarly, on Julia nightly (1.12.0-DEV.339 / b9aeafa), the MWE is broken in the same way: (click to expand)
  | | |_| | | | (_| |  |  Version 1.12.0-DEV.339 (2024-04-13)
 _/ |\__'_|_|_|\__'_|  |  Commit b9aeafa171e (0 days old master)

julia> using Test: @test_throws

julia> module MyModule end
Main.MyModule

julia> foo() = MyModule.x
foo (generic function with 1 method)

julia> @test_throws UndefVarError(:x) foo()
ERROR: UndefRefError: access to undefined reference
Stacktrace:
 [1] do_test_throws(result::Test.ExecutionResult, orig_expr::Any, extype::Any)
   @ Test ~/.asdf/installs/julia/nightly/share/julia/stdlib/v1.12/Test/src/Test.jl:815
 [2] top-level scope
   @ REPL[4]:1

The issue here is that the behavior of the UndefVarError(::Symbol) constructor has changed between 1.10.2 and 1.11.0-beta1.

The UndefVarError(::Symbol) constructor is in the manual. So, some questions are:

  1. Do we think that the behavior of the UndefVarError(::Symbol) constructor is public API?
  2. Do we think this change is a breaking change?
  3. Is there a way we can patch @test_throws to avoid this specific use case from breaking?

My initial thought is that this isn't a breaking change, and that we don't need to fix it in Julia; instead, we can just tell users to fix their code, e.g.:

@@ -1,7 +1,11 @@
 using Test: @test_throws

 module MyModule end

 foo() = MyModule.x

-@test_throws UndefVarError(:x) foo()
+if Base.VERSION >= v"1.11-"
+    @test_throws UndefVarError(:x, MyModule) foo()
+else
+    @test_throws UndefVarError(:x) foo()
+end

But I'm curious to hear what other people think.

@DilumAluthge
Copy link
Member Author

If we do decide that we want to fix this in Julia itself, here are two possible ways to fix it: #54083, #54084

@Keno
Copy link
Member

Keno commented Apr 14, 2024

I prefer #54084

@nickrobinson251
Copy link
Contributor

I think this was a breaking change, and should have been on the v1.11 milestone, fwiw.

Over in ReTestItems.jl i was using exactly this to test that an UndefVarError is thrown for a specific variable name in a test. I think UndefVarError(:x) was public API (since it was in the manual), so @test_throws UndefVarError(:x) foo() was valid usage that worked on v1.10 and now fails on v1.11.

I'm don't think either of the PRs linked above fix the MWE.

I'm not sure i see another fix than special-casing UndefVarError in @test_throws... so if i'm the only one using this i'm happy enough to just go change the usage in ReTestItems.jl to add a VERSION check

But thought it was worth bumping this again since i think this was technically breaking (and might affect others)

@nickrobinson251
Copy link
Contributor

nickrobinson251 commented Oct 17, 2024

hm, actually, i'm not sure i know how to fix this in the package. How can i test that an UndefVarError(:x) was thrown from a module with a gensym'd name (i.e. when i don't know the module)?

I don't want to test that any UndefVarError was thrown, e.g. @test_throws UndefVarError foo() since that's liable to pass incorrectly based on a typo (i.e. not the UndefVarError i'm intending to test for, e.g. suppose my function is actually named foo2()).

Nor do I want to test based on the printed output, e.g. @test_throws "UndefVarError: x not defined" foo(), since that's fragile and liable to fail with every minor Julia release (since we can and do change printing in minor releases), so support multiple Julia versions in CI will make the test into a big if ... else block of version checks.

advice appreciated!

otherwise i think we should go fix this in @test_throws

@DilumAluthge
Copy link
Member Author

DilumAluthge commented Oct 17, 2024

You could fix it in a package with something like this:

using Test: @test

function get_exception(f::F) where {F <: Function}
    caught_exception = false
    ex = try
        f()
    catch ex
        caught_exception = true
        ex
    end
    if !caught_exception
        error("No exception was thrown")
    end
    return ex
end

function test_throws_undefvarerror(f::F, expected_var) where {F <: Function}
    return @test get_exception(f).var == expected_var
end

And then example usage would look like this:

function f()
    return foo_bar
end

function g()
    return hello_world
end

test_throws_undefvarerror(f, :foo_bar) # passes

test_throws_undefvarerror(g, :foo_bar) # fails

@vtjnash
Copy link
Member

vtjnash commented Oct 18, 2024

The test_throws macro returns the exception object also, so that you can do more tests on it too

@DilumAluthge
Copy link
Member Author

Ah, so then the example can be simplified to:

function f()
    return foo_bar
end

function g()
    return hello_world
end

ex = (@test_throws UndefVarError f()).value
@test ex.var == :foo_bar # passes

ex = (@test_throws UndefVarError g()).value
@test ex.var == :foo_bar # fails

@nickrobinson251
Copy link
Contributor

nickrobinson251 commented Oct 18, 2024

thanks for the suggestion!

I think

@test_throws UndefVarError(:x) f()

is a much nicer and more obvious public API for this than either of those (especially since fields are not usually considered public), so i think this has convinced me we should keep supporting it -- will put up a PR

giordano pushed a commit that referenced this issue Oct 19, 2024
…st_throws (#56231)

Fix #54082 

Arguably this was a breaking change (as a consequence of
#51979). But regardless, it seems
like useful functionality to have a public API for testing that an
`UndefVarError` was thrown for the expected variable name (regardless of
scope). This is particularly useful if you don't know what the scope is
(for example, in my use-case i want to test that a specific
`UndefVarError` is thrown from a module with a `gensym`'d name).

Pre-v1.11 the syntax for this was 
```julia
@test_throws UndefVarError(:x) foo()
```
but that stopped working in v1.11 when `UndefVarError` got a second
field (in fact in v1.11.1 this is an error when before it would pass)

This PR restores that functionality.

We might want to backport it to v1.11.x so that v1.11 isn't the only
version that doesn't support this.
giordano pushed a commit that referenced this issue Oct 19, 2024
…st_throws (#56231)

Fix #54082

Arguably this was a breaking change (as a consequence of
#51979). But regardless, it seems
like useful functionality to have a public API for testing that an
`UndefVarError` was thrown for the expected variable name (regardless of
scope). This is particularly useful if you don't know what the scope is
(for example, in my use-case i want to test that a specific
`UndefVarError` is thrown from a module with a `gensym`'d name).

Pre-v1.11 the syntax for this was
```julia
@test_throws UndefVarError(:x) foo()
```
but that stopped working in v1.11 when `UndefVarError` got a second
field (in fact in v1.11.1 this is an error when before it would pass)

This PR restores that functionality.

We might want to backport it to v1.11.x so that v1.11 isn't the only
version that doesn't support this.

(cherry picked from commit b0c1525)
KristofferC pushed a commit that referenced this issue Oct 21, 2024
…st_throws (#56231)

Fix #54082 

Arguably this was a breaking change (as a consequence of
#51979). But regardless, it seems
like useful functionality to have a public API for testing that an
`UndefVarError` was thrown for the expected variable name (regardless of
scope). This is particularly useful if you don't know what the scope is
(for example, in my use-case i want to test that a specific
`UndefVarError` is thrown from a module with a `gensym`'d name).

Pre-v1.11 the syntax for this was 
```julia
@test_throws UndefVarError(:x) foo()
```
but that stopped working in v1.11 when `UndefVarError` got a second
field (in fact in v1.11.1 this is an error when before it would pass)

This PR restores that functionality.

We might want to backport it to v1.11.x so that v1.11 isn't the only
version that doesn't support this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment