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

Deprecate conflicting @testset arguments #55174

Merged
merged 2 commits into from
Aug 6, 2024

Conversation

mhauru
Copy link
Contributor

@mhauru mhauru commented Jul 19, 2024

Currently @testset allows specifying multiple descriptions and testset types, and only the last one will take effect. The others will be silently ignored.

This PR starts printing deprecation warnings whenever such conflicting arguments are provided.

Ideally I would also propose fixing the order in which the arguments must be provided (currently order doesn't matter), but I've left that out at least for now since that's a more breaking change. Can put it in though if others are for it.

@nsajko nsajko added the testsystem The unit testing framework and Test stdlib label Jul 22, 2024
Copy link
Contributor

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the idea, thank you!

Unfortunately the tests in here don't seem to work as intended? At least they cause the CI tests to fail:

Error During Test at /cache/build/tester-amdci4-12/julialang/julia-master/julia-33da5bdd9b/share/julia/stdlib/v1.12/Test/test/runtests.jl:1734
  Test threw exception
  Expression: #= /cache/build/tester-amdci4-12/julialang/julia-master/julia-33da5bdd9b/share/julia/stdlib/v1.12/Test/test/runtests.jl:1734 =# @macroexpand #= /cache/build/tester-amdci4-12/julialang/julia-master/julia-33da5bdd9b/share/julia/stdlib/v1.12/Test/test/runtests.jl:1734 =# @testset("name1", "name2", begin
        end)
  Multiple descriptions provided to @testset. This may be disallowed in the future.
  Stacktrace:
    [1] _depwarn(msg::Any, funcsym::Any, force::Bool)
      @ Base ./deprecated.jl:241
...
Error During Test at /cache/build/tester-amdci4-12/julialang/julia-master/julia-33da5bdd9b/share/julia/stdlib/v1.12/Test/test/runtests.jl:1737
  Test threw exception
  Expression: #= /cache/build/tester-amdci4-12/julialang/julia-master/julia-33da5bdd9b/share/julia/stdlib/v1.12/Test/test/runtests.jl:1737 =# @macroexpand #= /cache/build/tester-amdci4-12/julialang/julia-master/julia-33da5bdd9b/share/julia/stdlib/v1.12/Test/test/runtests.jl:1737 =# @testset(DefaultTestSet, DefaultTestSet, begin
        end)
  Multiple testset types provided to @testset. This may be disallowed in the future.

@LilithHafner
Copy link
Member

This is a great idea; definitely worth implementing. CI currently fails because we run tests with --depwarn=error, so these warnings via Base.depwarn are errors instead. Quick exploration locally seems to indicate the implementation is good.

@mhauru
Copy link
Contributor Author

mhauru commented Aug 6, 2024

Tests pass for me locally indeed without --depwarn=error. I can convert the @test_logs calls to @test_throws and try to get tests to pass that way.

@LilithHafner
Copy link
Member

Then Base.runtests() will fail unless --depwarn=error is set which will pass CI, but be bad for folks doing manual testing.

You're looking for @test_deprecated

@LilithHafner
Copy link
Member

Thank you! I appreciate this deprecation.

@LilithHafner LilithHafner merged commit 22f5580 into JuliaLang:master Aug 6, 2024
7 checks passed
lazarusA pushed a commit to lazarusA/julia that referenced this pull request Aug 17, 2024
Currently `@testset` allows specifying multiple descriptions and testset
types, and only the last one will take effect. The others will be
silently ignored.

This PR starts printing deprecation warnings whenever such conflicting
arguments are provided.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testsystem The unit testing framework and Test stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants