Junit4: Undeprecate ExpectedException rule

[sbrannen]

After upgrading the Spring Framework to JUnit 4.13 beta 3, I noticed that org.junit.rules.ExpectedException is now @Deprecated, and this change generates a lot of warnings in a large code base like that.

Since 4.13 is the last intended release in the 4.x line, I do not think it makes sense to deprecate such a commonly used and supported feature.

If you keep the deprecation in place, I fear that it will only annoy thousands (millions?) of developers for no good reason.

[kcooney]

I think the deprecation warning is a good way to indicate that something better is available, so the warnings aren't there for no good reason. ExpectedException can easily be misused yet is still recommended in a lot of places.

The question is how problematic it is for our users to have these deprecation warnings. In my company deprecation warnings don't show up at build time (only when viewing code).

[marcphilipp]

I think Sam has a point here. I've seen many builds that forbid all (new) compiler warnings. Such projects would have to go through all their test classes that use ExpectedException and add a SuppressWarnings annotation.

@stefanbirkner WDYT?

[kcooney]

Having clients suppress the warnings seems reasonable. They could also suppress all deprecation warnings; treating deprecation warnings as errors makes it impossible to deprecate anything.

Deprecation is our best way to indicate to people that the old API is problematic and much better APIs are available.

[sbrannen]

I understand that developers can suppress warnings locally within an IDE; however, I disagree that deprecating a core feature in the (planned-to-be) final release of JUnit 4.x is a good idea.

IMHO, it is not a good idea to deprecate a core feature in the final release of any framework, especially one in such widespread use as JUnit 4.

As a concrete example, in the core Spring Framework test suite we now have thousands of deprecation warnings for the use of the ExpectedException rule.

The only options we have are:

1. Suppress deprecation warnings in every single test class that uses ExpectedException.
2. Migrate away from the use of ExpectedException in every single test class that uses it.
3. Do nothing.

If we go with # 1, we can likely achieve that with some scripting that suppresses warnings at the class-level, but that would then suppress deprecation warnings of other APIs that we likely want/need to know about and not ignore. Otherwise, we have to manually go through each test class and deprecate each use of the ExpectedException API, and that includes every single single interaction with the rule instance in all affected test methods.

If we go with # 2, that is a major undertaking that might take several days to accomplish manually. Partial automation may be possible, but the investment in the automation (scripting, etc.) may be rather extensive on its own.

If we go with # 3, we then have unnecessary (from our perspective) deprecation warnings that cloud our view of deprecation warnings we actually care about, and that leads to the Broken Window effect, which we want to avoid.

[panchenko]

Doing # 2 would simplify future migration to JUnit 5.

[sbrannen]

Doing # 2 would simplify future migration to JUnit 5.

Sure. That would make a migration to JUnit Jupiter or AssertJ potentially easier; however, not everyone wants to or needs to migrate existing test suites.

Thus, I do not consider that a valid argument.

[kcooney]

I'm not sure why this likely being the last JUnit 4.x release is related to whether or not we deprecate things. JUnit rarely deletes _any_ APIs, so deprecation hasn't been an indication that an API is going away. It's an indication that either that the API might not be supported and/or better APIs exist. Both are true in this case (we've rejected proposed changes to this rule because we think it's better to just have people use assertThrows())

I suggest Spring slowly migrate away from ExpectedException. A few days of work spread out over a few months to get to safer and easier to understand tests seems like a win. The benefit is other people less familiar with recent changes to JUnit will know to avoid ExpectedException

ErrorProne has a check for this that will provide a recommended fix, so I don't think it would take several days to resolve this issue.

[stefanbirkner]

TL;DR I'm in favor for deprecating ExpectedException.

@kcooney already said that deprecation in JUnit 4 does not mean that we will remove things because a design decisions of JUnit 4 is to avoid breaking changes. Therefore it does not so important whether this is the last 4.x release or not.

I think it is helpful to have the deprecation because it tells people that there is a better way of checking exceptions. I worked for a few companies for the last 10 years and always there are people who are suprised that there is something better than @Test(expected=ABeautifulException.class. Deprecation is valuable for them because they usually have a look when they see code that is striked through in the IDE.

The deprecation warnings may be annoying because there is no need to fix these warnings. The origin of this is the way how JUnit 4 uses deprecation. Therefore I think there is no fix for this apart from not deprecating ExpectedException (and finally not using deprecation at all).

Last but not least there are companies whose builds break in case of deprecation warnings and therefore they cannot easily upgrade to JUnit 4.13. I'm not sure how much of these companies exist, which are companies that a) have this build policy and b) want to upgrade to JUnit 4.13.

@sdeleuze, @mp911de and @rweisleder you voted for reverting the deprecation. It would be helpful if you tell us why you want it to not be deprecated.

[philwebb]

Just for a bit more information, it took about 1.5 days to migrate Spring Framework's codebase from JUnit's ExpectedException to AssertJ's assertThatExceptionOfType.

One small suggestion that might help is to consider deprecating just the ExpectedException.none() method rather than the entire class. This would still give people the warning, but it would make it much easier to suppress in isolation.

[mp911de]

JUnit's ExpectedException is used as a primitive type in tests that perform more extensive exception assertions than just @Test(expected = …). These are typically tests that rely on Hamcrest or built-in assertions and not so much AssertJ or that like.

Having JUnit 4.13 is a signal of active maintenance happening. Code bases that remain on JUnit 4.x API are likely to not be migrated to JUnit 5 API soon but rather they keep the API with potentially migrating to the Vintage engine. Having a suddenly deprecated class generated new warnings and additional effort to migrate to a different assertion utility although the code might still be working.

I've observed deprecations and introduction of improved API a few times. In most cases, deprecation helped maintainers and not actual users. On the user side, this generated mostly effort with no benefit.

[sbrannen]

One small suggestion that might help is to consider deprecating just the ExpectedException.none() method rather than the entire class. This would still give people the warning, but it would make it much easier to suppress in isolation.

If the JUnit 4 team decides not to undeprecate the ExpectedException rule, I think this would be a good compromise.

[rweisleder]

I wonder if @Test(expected = ...) should also be deprecated since we have assertThrows now. Or at least the reference to ExpectedException should be removed from the attribute's Javadoc.

[sbrannen]

I wonder if @Test(expected = ...) should also be deprecated since we have assertThrows now. Or at least the reference to ExpectedException should be removed from the attribute's Javadoc.

Yes, the expected attribute in org.junit.Test should likely be deprecated. In any case, the class-level Javadoc for org.junit.Test and the expected attribute should be updated to recommend the use of assertThrows.

[kcooney]

I would be in favor of deprecating just ExpectedException.none().

[proski]

I believe using deprecation to advertise a new feature is a heavy handed approach that would damage trust in JUnit development process and hold back adoption of version 4.13.

There should be a version overlap when the old and the new feature coexist without either being deprecated. As others pointed out, having dependencies on deprecated methods is a no-go for many projects that keep code quality high. That means the JUnit upgrade should be in the same commit as the changes to migrate the whole codebase.

I believe the only situation when a feature is deprecated and the replacement is added in the same release is when the feature is fundamentally broken and should be migrated away immediately. I don't think ExpectedException is fundamentally broken.

I would often check newer dependencies to see what changes are coming and how I can prepare my code. So, how do I get my code ready for the JUnit 4.13 migration? I know that ExpectedException will be deprecated, but I don't have a good replacement yet. I would need to implement my own code to catch and check exceptions to avoid that mess altogether.

Just because 4.13 is planned to be the final 4.x release, it's not a good reason to abandon sane software development practices. It fact, 4.13 can be released without the ExpectedException deprecation and 4.14 can be released the next day with the deprecation. That would get the message across while giving developers a little breather to do the migration without the undue pressure.

[marcphilipp]

There should be a version overlap when the old and the new feature coexist without either being deprecated.

Why? The intent of the deprecation is to make people aware of the new, in our opinion better API.

I also agree that we should only deprecate ExpectedException.none() to make it less invasive.

@stefanbirkner Ok with you?

[proski]

@marcphilipp When I update my code, I want things to work without deprecations unless I'm doing something wrong. It's too intrusive to deprecate a feature and provide a replacement at the same time. Instead, the 4.13 release notes should say that ExpectedException will be deprecated in the next version and suggest a replacement. That way, I'll have a way to migrate my code step-by-step without having to allow deprecations in the meantime. That's just basic courtesy to the users who have been using JUnit 4.12 for years and don't want big changes.

[marcphilipp]

IMHO the point of a deprecation warning _is_ to give you time to migrate your code towards the replacement. If you need to work around the deprecation warning, you can create your own temporary factory method that delegates to ExpectedException.none().

[proski]

OK, the PR mitigates the issue to some degree, it's definitely a step in the right direction. Thank you for doing that!

Rather than suggesting that users implement a factory method, the code could provide its own non-deprecated method (maybe none with an extra argument).

I still don't understand the resistance against giving users a transitional version. When implementing an incompatible change, it's very common to make a version that allows transition without forcing it in any way.

Without a transitional version, users would try 4.13, see tons of warnings and go back to 4.12.

With a transitional version, users would try 4.14, see tons of warnings, try 4.13, see no warnings, use it. Some would update to 4.14 quickly, other would happily stay with 4.13 unless they have a good reason to update. It should be fine either way.

[hjohn]

I also don't understand why this deprecation should have happened in JUnit 4. I'm migrating to JUnit 5 using the junit-vintage-engine to support both versions temporarily. But this pulls in JUnit 4.13 which for some reason decided that it was a good idea to deprecate things (that have been supported for years) in what is likely one of its last releases.

There also seems to be no reason for the deprecation other than to point out an alternative -- this could have been achieved by at least offering an alternative (make the constructor public, or offer another static method -- message communicated).

The ExpectedException code as it is now will continue working (and will keep working even in later releases) as it simply uses basic features of JUnit offered in an (at the time) convenient way.

[daicoden]

What's the replacement for asserting properties about the exception...

old code:

thrown.expect(StatusRuntimeException.class);
thrown.expect(its(StatusRuntimeException::getStatus, toBe(Status.INVALID_ARGUMENT)));
GrpcDispatch.makeRequest(service::enableJob, JobReference.newBuilder().setName(UNKNOWN_JOB).build());

Seems like assertThrows is missing a matcher version to allow expected exception to be fully deprecated.... I know it's too late but no one else has this problem?

[miralexan]

@daicoden

I'm pretty sure assertThrows returns the throwable, so you could just capture it and make assertions like you would for any other object:

StatusRuntimeException thrown = assertThrows(StatusRuntimeException.class, () -> {
   GrpcDispatch.makeRequest(service::enableJob, JobReference.newBuilder().setName(UNKNOWN_JOB).build());
});

assertEquals(Status.INVALID_ARGUMENT, thrown.getStatus());