refactor(internal): deprecate the implementations of deprecated contracts - #3423
Open
juherr wants to merge 1 commit into
Open
refactor(internal): deprecate the implementations of deprecated contracts#3423juherr wants to merge 1 commit into
juherr wants to merge 1 commit into
Conversation
…acts IClass deprecated getInstances, getInstanceHashCodes and addInstance in 7.10.0, but TestClass, ClassImpl and NoOpTestClass carried on implementing them unmarked. Every implementation, and every call delegating to the next one down, therefore read as a fresh use of a deprecated API. Marking the implementations states what is already true of the contract they serve. getInstanceHashCodes is the awkward one: the same body also implements IObject.getInstanceHashCodes, which is current and still reached through ITestNGMethod. A comment at each of the three sites says so, because the annotation on its own would read as if the whole method were dead. The annotated types are package-private or live in org.testng.internal, which javadoc already excludes, so no published API changes shape. TestNG also read two defaults off CommandLineArgs, a class its own javadoc schedules for removal in 8.0. Those values are runner defaults rather than CLI ones -- the CLI overwrites both unconditionally -- so TestNG now declares them itself alongside DEFAULT_OUTPUTDIR. The main source set ends up free of deprecation warnings without a single suppression.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change marks legacy instance APIs as deprecated and adds local TestNG defaults for the test JAR XML path and suite thread-pool size. Existing method behavior remains unchanged. ChangesLegacy instance API deprecations
TestNG default values
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This change marks deprecated implementations and moves runner defaults away from a deprecated command-line class without changing intended behavior; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
IClassdeprecatedgetInstances,getInstanceHashCodesandaddInstancein 7.10.0, butTestClass,ClassImplandNoOpTestClasswent on implementing them unmarked. javac thereforereported every implementation, and every call delegating to the next one down, as a fresh use of a
deprecated API — thirteen warnings for a decision that was taken years ago.
Marking the implementations states what is already true of the contract they serve. It also silences
the delegating calls in their bodies for free, since a deprecated method may use deprecated API.
TestNGseparately read two defaults offCommandLineArgs, whose own javadoc schedules it forremoval in 8.0. It now declares them itself.
What to look at in review
getInstanceHashCodesis the one that needs a decision. The same method body satisfies twocontracts:
IClass.getInstanceHashCodes()— deprecated since 7.10.0IObject.getInstanceHashCodes()— current, and still reached from the non-deprecatedITestNGMethod.getInstanceHashCodes()by way ofBaseTestMethodandIObject.instanceHashCodesSo
@Deprecatedon those three implementations is half true. Each carries a comment saying whichhalf, because the annotation alone reads as if the whole method were dead — and
org.testng.internalis an exported OSGi package, so a downstream holding a
NoOpTestClassnow sees a deprecation on amethod whose
IObjectcontract is current.The clean fix is to stop the two interfaces sharing a name, and #3424 now does exactly that: it
renames
IObject.getInstanceHashCodes()togetObjectHashCodes(), after which these annotations aresimply true and the three comments go away. It is stacked on this branch, so this PR still reads on
its own; #3424 also turned up a nullness gap in
ClassImplthat the shared name had been hiding.The two
TestNGdefaults are runner defaults, not CLI defaults.CliConfigureroverwrites bothunconditionally (
setXmlPathInJar,setSuiteThreadPoolSize), so the CLI path never observes thesefield initializers; they exist for the programmatic path, where
m_xmlPathInJarfeedsJarFileUtilsandm_suiteThreadPoolSizepicks the sequential-versus-parallel branch. Reading themoff a class scheduled for deletion was the compiler pointing that out. They are now declared next to
DEFAULT_OUTPUTDIR, private likeDEFAULT_THREADPOOL_FACTORY. Say the word if you would ratherthey were public, so callers keep a supported way to read the default after 8.0.
The remaining
CommandLineArgsuses inTestNGall sit inside@Deprecatedmethods and arealready silent.
Scope
The annotated types are package-private (
TestClass) or live inorg.testng.internal, whichjavadoc already excludes. No published API changes shape, and no behaviour changes.
Effect on the build's warning output
Counters read from
./gradlew testClasses --rerun-taskswith Error Prone and autostyle switchedoff, so the numbers are javac's alone:
warning: [deprecation]warning: [removal]The thirteen this removes are every deprecation warning the
mainsource set produced. Theremainder are in test sources and are back-compatibility coverage — tests that exercise a deprecated
API on purpose, which should keep doing so.
Verification
./gradlew buildpasses on this branch: no failures and no errors across the test result files.Summary by CodeRabbit
Deprecations
Configuration