-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Undeprecate ExpectedException rule #1609
Comments
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. 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). |
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 @stefanbirkner WDYT? |
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. |
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 The only options we have are:
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 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. |
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. |
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 I suggest Spring slowly migrate away from 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. |
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 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. |
Just for a bit more information, it took about 1.5 days to migrate Spring Framework's codebase from JUnit's One small suggestion that might help is to consider deprecating just the |
JUnit's 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. |
If the JUnit 4 team decides not to undeprecate the |
I wonder if |
Yes, the |
I would be in favor of deprecating just |
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 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 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 |
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 @stefanbirkner Ok with you? |
@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 |
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 |
Instead of deprecating the whole class, now only the factory method is deprecated and points to `assertThrows()`. This will make it easier to temporarily suppress the exception for codebases that use it extensively by adding an intermediate factory method of their own. Resolves #1609.
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 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. |
Instead of deprecating the whole class, now only the factory method is deprecated and points to `assertThrows()`. This will make it easier to temporarily suppress the exception for codebases that use it extensively by adding an intermediate factory method of their own. Resolves #1609.
I also don't understand why this deprecation should have happened in JUnit 4. I'm migrating to JUnit 5 using the 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 |
What's the replacement for asserting properties about the exception... old code:
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? |
I'm pretty sure StatusRuntimeException thrown = assertThrows(StatusRuntimeException.class, () -> {
GrpcDispatch.makeRequest(service::enableJob, JobReference.newBuilder().setName(UNKNOWN_JOB).build());
});
assertEquals(Status.INVALID_ARGUMENT, thrown.getStatus()); |
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.
The text was updated successfully, but these errors were encountered: