@test_deprecated macro

[StefanKarpinski]

See discussion here: https://github.com/JuliaLang/julia/pull/20330/files#r98575767. I wanted to test that something was deprecated, which is complicated by the fact that the behavior of deprecation can be changed by the --depwarn={no|yes|error} flag to julia. My solution there was to check Base.JLOptions().depwarn and do different tests depending on that value. We could have a @test_deprecated macro that generates the correct underlying test depending on that flag: for --depwarn=no it emits a standard test to check that the deprecation definition works, for --depwarn=yes it emits @test_warn "deprecated" test to make sure that the deprecation warning is emitted correctly, and for --depwarn=error it checks that the given expression raises an ErrorException with the word "deprecated" in its message. One of the major benefits of testing for this is so that when you delete a deprecation, you can't forget to update the tests to check for an unconditional error, which is what I originally wanted to do.

[ararslan]

💯 I've been wanting something like this for quite a while. It will also (hopefully) help ensure that deprecations are done properly. It's easy to mess those up, so having a way to check would be fantastic.

[yuyichao]

I think ideally this should hook slightly deeper to the depwarn system to actually check if a depwarn is triggered. Conditional changing to test/test_warn/test_error has the issue that it can be confused with other warning/errors.

[StefanKarpinski]

Sure, even better integration would obviously be even better.

[tkelman]

as I said there, if this is going to be testing for 3 different possibilities then these tests should be put in a file that gets run with all 3 settings, depwarn_exec or similar like we do for bounds checking

[JeffBezanson]

I don't get this. Every time we add a deprecation we should test that the thing is in fact deprecated?

[ararslan]

@JeffBezanson I see it as just an added a safeguard against misspecifying, omitting, or failing to remove a deprecation.

As an aside, we could have deprecated specified separately in choosetests so that you can run julia test/runtests.jl deprecated locally without having to add to the CI time.

[yuyichao]

I think it's mostly because we've had many cases where the deprecation doesn't actually work because there's no test for it. Adding a test macro that suppresses the depwarn can make sure such thing can still be tested without printing pages of depwarn/slow down due to backtrace collection in depwarn. I don't really think we need to make sure each depwarn behaves correctly at all three cmdline settings although a independent test for that can be useful.

[StefanKarpinski]

In my case, it was mostly so that I could write tests now that will fail when we remove the deprecation, reminding me (or whoever) to actually go fix the tests to test that the post-deprecation behavior is correct. Otherwise you have some todo in a test file that will be ignored for eternity.

[tkelman]

A lot of deprecations get written incorrectly and not fixed until some package breaks because of it. With PkgEval completely freezing and not finishing for the whole month, we're not catching these systematically right now.

[martinholters]

The --depwarn=yes case could also check the correctness of the result. Then this would actually be sufficient to test the deprecation, the other cases merely testing the deprecation mechanism as such.

And 💯 for testing deprecations! Too many bugs discovered too late...

[StefanKarpinski]

So, ideally, we could do a single test run that tests that the expression does in fact trigger a deprecation warning and that the deprecated behavior is the expected one. Since we generally already had tests for deprecated functions in place, we could just leave a few around and they would automatically get cleaned out when the deprecation is deleted (since otherwise tests fail).

[omus]

I'm in favor of being able to test deprecations. An example of an incorrect deprecation that needed fixing: #19375

[ararslan]

Since we generally already had tests for deprecated functions in place, we could just leave a few around and they would automatically get cleaned out when the deprecation is deleted (since otherwise tests fail).

Regarding this, when something is deprecated perhaps we can move the existing tests for it (or at least a couple) to something like a test/deprecated.jl rather than leaving them in the same place. (Is the latter what you meant?) That would make housekeeping easier when deprecations are removed.

[StefanKarpinski]

Well, in this case, unlike with deprecations which will be deleted, the tests will remain when the deprecations are deleted but they'll need to be updated, so I'd rather leave them in their place.

[c42f]

I just stumbled across this issue. It should be really easy to sort this out once #24490 is merged - the depwarn now comes out as a structured log record including attaching the caller as a StackFrame and whatever other information we find necessary to transport.

Regarding the behavior depending on the value of --depwarn, I'd actually like to completely rework this option. I'd like to

1. Remove --depwarn=error because we'll be able to get the same behavior in a much more granular way by just installing a custom logger.
2. Map the current --depwarn={yes,no} to a loglevel (yes->Warn; no->BelowMinLevel or anything below or equal to the default disable_logging threshold. This should give the same performance characteristics as the current --depwarn=no.

I'm assuming that performance is the reason for --depwarn=no to exist at all. If it's mainly convenience, we can just remove the whole thing and let users control the depwarn behavior themselves by installing a logger which ignores everything with the group=:depwarn key.

[StefanKarpinski]

Note that in the 1.x series we may want to have --depwarn=no be the default and have people opt into --depwarn={yes,error} so that we can deprecate certain constructs without violating the 1.x contract that your 1.x code will work in 1.y for y ≥ x. That way we can continue to evolve the language with opt-in deprecations instead of the current default of opt-out deprecations. Of course, we may also want more fine-grained mechanisms. Implementing this in terms of log levels may make sense but I suspect that having dedicated command-line options for it may still make sense.

[c42f]

Right, that's an interesting point about having a --depwarn=no default. It's true that with a specialized mechanism we do have more control. For example we could also make using deprecated functions completely free at runtime if it's acceptable to have some recompilation when changing away from the default depwarn (presumably this wouldn't affect the sysimg, as Base would always be depwarn free).

But not to derail this issue - more on-topic, I've now implemented a @test_deprecated macro as part of porting the existing tests in #24490, but I'm wondering about the actual semantics we'd like to have with regard to testing the return value in addition to testing any logging. The simplest is not to bother with the return value, and to require a separate test for that:

@test_deprecated num2hex(1)
@test num2hex(1) == "0000000000000001"

Another option is to have an analog of @inferred which requires an additional @test to check the return value.

@test @deprecated(num2hex(1)) == "0000000000000001"

A further more opinionated option would be to require that the test expression return true, in addition to testing that a depwarn is generated:

@test_deprecated num2hex(1) == "0000000000000001"

Finally, we can do it like @test_warn and return the value, so that the tests can be nested if desired:

@test @test_deprecated(num2hex(1)) == "0000000000000001"

In the process of writing the above list, I've come to the conclusion that the @test_warn approach is best, because

• It registers the deprecation part of the test as its own distinct test in the test set
• It allows the user to not care about the return value, so it's strictly more functional than the first design on the list, with the same ease of use
• It can be used stand alone - unlike @inferred which doesn't register itself with the test set but requires an outer @test to do that. (As a heavy user of @inferred, I guess this is a bit of a grip of mine.)

[StefanKarpinski]

Perhaps a separate issue about changing @inferred to @test_inferred is warranted?

[c42f]

Agreed. See #24829 for @inferred discussion.

[c42f]

This is addressed now that #24490 is merged.

The only caveat I've found is that the use of --depwarn=error in the standard tests means that the form

@test @test_deprecated(num2hex(1)) == "0000000000000001"

doesn't actually work until we remove --depwarn=error and replace it with something which defers the exception until later. I've added this to my list of logging-related cleanup - I think the only tricky bit will be getting @deprecate_binding to emit a normal log event. See #25109

[c42f]

Someone with committer permissions will have to close this for me ;-)

[Sacha0]

Thanks @c42f! :)