-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add errorprone #2958
Add errorprone #2958
Conversation
@krmahadevan As suggested by @vlsi , I added errorprone only in a single modification. |
suiteOneTestOneTestMethodLevelEventLogs.stream() | ||
.filter(e -> e.getEvent() == TestNgRunEvent.LISTENER_TEST_METHOD_START); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out these lines were doing nothing, however, it is not clear what was the intention.
The code has been added in https://github.com/testng-team/testng/pull/1910/files#diff-af16c31109bb20a887788e673417872d9ce0789634234868c6bcfaa1c13203b6R92, and it was not modified since.
@MicahLC, do you by chance remember what was the intention of adding event filtering of "event == listener_test_method_start" here? It looks like result of the stream was not later, so the code looks like a dead code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vlsi Exactly. I have to investigate what are the potential side effects. The alternative will be easy: just remove the dead code :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Removing dead code is one of the approaches. However, it might signal the test is not doing something expected.
What do you think of removing the dead code, and creating an issue to investigate if it should be re-added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I will consider it when it will be the last failing test :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How dare you make me revisit my code from 5 years ago 😄
This was probably an oversight on my part, I think the stream/filter here isn't necessary. On line 268 of this file (as written), it checks e.getEvent() == TestNgRunEvent.LISTENER_TEST_METHOD_PASS
, and there won't be any events of that type if you filter by e.getEvent() == TestNgRunEvent.LISTENER_TEST_METHOD_START
here.
If I had to guess, I probably had some logging there originally and accidentally left this line in.
490010d
to
14cb1ad
Compare
14cb1ad
to
2632ff5
Compare
2632ff5
to
af6f1d6
Compare
WalkthroughThe recent updates focus on enhancing code quality and efficiency across the project. Key changes include the introduction of the Error Prone static analysis tool to improve code reliability, optimizations in data structure usage for better performance, and the standardization of assertions in test cases for clearer, more consistent testing outcomes. These adjustments aim to streamline development workflows, reduce potential errors, and maintain high standards of code quality. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
@juherr - There are a lot of unrelated changes in this PR. Can you please check ? |
@krmahadevan why do you think that? It is just fix of warnings. I agree I should group them in 1 commit by violation. |
I thought i saw some formatting changes as well. The warnings changes as a separate PR would have been easy to merge because it shouldnt require any serious review but the error prone changes may need some time to look at. Its ok if they cant be split. |
1915c56
to
23371c7
Compare
@krmahadevan There is still many warnings but we can merge once you'll reviewed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (52)
- build-logic/build-parameters/build.gradle.kts (1 hunks)
- build-logic/code-quality/build.gradle.kts (1 hunks)
- build-logic/code-quality/src/main/kotlin/testng.errorprone.gradle.kts (1 hunks)
- build-logic/jvm/src/main/kotlin/testng.java.gradle.kts (1 hunks)
- testng-asserts/src/test/java/org/testng/AssertTest.java (2 hunks)
- testng-core/src/main/java/org/testng/SuiteResult.java (2 hunks)
- testng-core/src/main/java/org/testng/internal/ClonedMethod.java (1 hunks)
- testng-core/src/main/java/org/testng/internal/Graph.java (3 hunks)
- testng-core/src/main/java/org/testng/internal/Tarjan.java (2 hunks)
- testng-core/src/main/java/org/testng/internal/annotations/JDK15TagFactory.java (1 hunks)
- testng-core/src/test/java/NoPackageTest.java (2 hunks)
- testng-core/src/test/java/test/ClassConfigurations.java (2 hunks)
- testng-core/src/test/java/test/CtorCalledOnce.java (2 hunks)
- testng-core/src/test/java/test/Exclude.java (2 hunks)
- testng-core/src/test/java/test/MethodTest.java (2 hunks)
- testng-core/src/test/java/test/NestedStaticTest.java (2 hunks)
- testng-core/src/test/java/test/SampleInheritance.java (2 hunks)
- testng-core/src/test/java/test/classgroup/Second.java (1 hunks)
- testng-core/src/test/java/test/configuration/issue2729/BeforeConfigTestSample.java (1 hunks)
- testng-core/src/test/java/test/custom/CustomAttributesTransformer.java (2 hunks)
- testng-core/src/test/java/test/dataprovider/issue2327/TestClassSample.java (2 hunks)
- testng-core/src/test/java/test/dataprovider/issue2565/SampleTestUsingFunction.java (1 hunks)
- testng-core/src/test/java/test/dataprovider/issue2565/SampleTestUsingPredicate.java (1 hunks)
- testng-core/src/test/java/test/dependent/BaseOrderMethodTest.java (2 hunks)
- testng-core/src/test/java/test/dependent/SampleDependentConfigurationMethods.java (2 hunks)
- testng-core/src/test/java/test/dependent/SampleDependentMethods.java (3 hunks)
- testng-core/src/test/java/test/dependent/SampleDependentMethods2.java (3 hunks)
- testng-core/src/test/java/test/dependent/SampleDependentMethods3.java (2 hunks)
- testng-core/src/test/java/test/factory/FactoryInSeparateClassTest.java (2 hunks)
- testng-core/src/test/java/test/factory/FactoryInterleavingTest.java (2 hunks)
- testng-core/src/test/java/test/hook/issue2251/SampleTestCase.java (1 hunks)
- testng-core/src/test/java/test/junitreports/JUnitReportsTest.java (2 hunks)
- testng-core/src/test/java/test/junitreports/TestSuiteHandler.java (1 hunks)
- testng-core/src/test/java/test/junitreports/Testsuite.java (2 hunks)
- testng-core/src/test/java/test/methods/SampleMethod1.java (2 hunks)
- testng-core/src/test/java/test/objectfactory/ContextAwareObjectFactoryFactory.java (2 hunks)
- testng-core/src/test/java/test/priority/parallel/EfficientPriorityParallelizationTest2.java (1 hunks)
- testng-core/src/test/java/test/sample/AfterClassCalledAtEnd.java (2 hunks)
- testng-core/src/test/java/test/sample/BaseAfterClassCalledAtEnd.java (1 hunks)
- testng-core/src/test/java/test/sample/BaseSampleInheritance.java (2 hunks)
- testng-core/src/test/java/test/sample/Basic1.java (2 hunks)
- testng-core/src/test/java/test/sample/Basic2.java (3 hunks)
- testng-core/src/test/java/test/sample/InvocationCountTest.java (2 hunks)
- testng-core/src/test/java/test/sample/JUnitSample2.java (1 hunks)
- testng-core/src/test/java/test/sample/PartialGroupVerification.java (2 hunks)
- testng-core/src/test/java/test/sample/Scope.java (2 hunks)
- testng-core/src/test/java/test/superclass/BaseSampleTest3.java (1 hunks)
- testng-core/src/test/java/test/thread/parallelization/BaseParallelizationTest.java (1 hunks)
- testng-core/src/test/java/test/tmp/Tmp.java (1 hunks)
- testng-core/src/test/java/test/triangle/Child1.java (2 hunks)
- testng-core/src/test/java/test/triangle/Child2.java (2 hunks)
- testng-runner-api/src/main/java/org/testng/internal/LiteWeightTestNGMethod.java (2 hunks)
Additional comments: 76
testng-core/src/test/java/test/superclass/BaseSampleTest3.java (2)
- 3-3: The addition of the
assertTrue
import statement is correct and necessary for the updated assertion method usage.- 10-10: Replacing
assert true;
withassertTrue(true);
correctly follows the PR's objective to enhance clarity and consistency in test cases using TestNG's assertion library.testng-core/src/test/java/test/triangle/Child2.java (2)
- 3-3: The import statement for
assertTrue
is correctly added to support the updated assertion method in the test case.- 16-16: The replacement of the
assert
statement withassertTrue
is appropriate, enhancing the test case's clarity and consistency with TestNG's assertion methods.testng-core/src/test/java/test/classgroup/Second.java (2)
- 3-3: The addition of the
assertTrue
import statement is correctly implemented to support the updated assertion method in the test case.- 12-12: Replacing the
assert
statement withassertTrue
correctly aligns with the PR's goal of using TestNG's assertion methods for improved clarity and consistency.build-logic/code-quality/build.gradle.kts (1)
- 10-10: Adding the
gradle-errorprone-plugin
dependency is correctly implemented, aligning with the PR's objective to integrate Error Prone into the project's build process.testng-core/src/test/java/test/sample/BaseAfterClassCalledAtEnd.java (2)
- 3-3: The addition of the
assertTrue
import statement is correctly implemented to support the updated assertion method in thebaseAfterClass
method.- 12-12: Replacing the
assert
statement withassertTrue
in thebaseAfterClass
method is appropriate and enhances the test's clarity and consistency.testng-core/src/test/java/test/sample/PartialGroupVerification.java (2)
- 3-3: The addition of the
assertTrue
import statement is correctly implemented to support the updated assertion method in the test case.- 15-17: Replacing the
assert
statement withassertTrue
in theverify
method is correctly done, enhancing the test's clarity and consistency with TestNG's assertion methods.testng-core/src/test/java/NoPackageTest.java (2)
- 1-1: The addition of the
assertTrue
import statement is correctly implemented to support the updated assertion method in theafter
method.- 17-17: Replacing the
assert
statement withassertTrue
in theafter
method is appropriate, enhancing the test's clarity and consistency with TestNG's assertion methods.testng-core/src/test/java/test/hook/issue2251/SampleTestCase.java (1)
- 17-17: Assigning the result of
new NullExObj().toString()
to aString
variableunused
is a valid change, but it's unclear if this modification serves a specific purpose related to the PR's objectives. It might be part of addressing a warning from Error Prone, but without context, it's hard to assess its relevance.Please clarify if this change is directly related to addressing an Error Prone warning or if it serves another purpose.
build-logic/code-quality/src/main/kotlin/testng.errorprone.gradle.kts (1)
- 1-15: The integration of the Error Prone plugin in the Gradle build script is correctly implemented. The use of the
errorprone
dependency and the configuration to enable Error Prone checks and disable warnings in generated code align with the PR's objectives to improve code quality.testng-core/src/test/java/test/triangle/Child1.java (3)
- 3-3: The addition of the
assertTrue
import statement is correctly implemented to support the updated assertion methods in thechild1
andchild1a
methods.- 15-15: Replacing the
assert
statement withassertTrue
in thechild1
method is appropriate, enhancing the test's clarity and consistency with TestNG's assertion methods.- 20-20: Similarly, replacing the
assert
statement withassertTrue
in thechild1a
method is correctly done, aligning with the PR's goal of using TestNG's assertion methods for improved clarity and consistency.testng-core/src/test/java/test/configuration/issue2729/BeforeConfigTestSample.java (1)
- 12-12: Adding the
@SuppressWarnings("ConstantOverflow")
annotation to suppress the "ConstantOverflow" warning for the division by zero operation is a specific change that likely addresses an Error Prone warning. This change is appropriate for the context of integrating Error Prone and addressing its warnings.testng-core/src/test/java/test/objectfactory/ContextAwareObjectFactoryFactory.java (2)
- 3-3: The addition of the
assertNotNull
import statement is correctly implemented to support the updated assertion method in thecreate
method.- 16-16: Replacing the
assert
statement withassertNotNull
in thecreate
method is appropriate, enhancing the method's clarity and consistency with TestNG's assertion methods.testng-core/src/test/java/test/sample/JUnitSample2.java (1)
- 28-28: Replacing a custom assertion with
assertNotNull
in thetestSample2ThatSetUpWasRun
method is correctly done, aligning with the PR's goal of using TestNG's assertion methods for improved clarity and consistency.testng-core/src/test/java/test/sample/Basic1.java (2)
- 3-3: The addition of the
assertTrue
import statement is correctly implemented to support the updated assertion method in thebasic1
method.- 26-26: Replacing the
assert
statement withassertTrue
in thebasic1
method is appropriate, enhancing the test's clarity and consistency with TestNG's assertion methods.testng-core/src/test/java/test/sample/Scope.java (3)
- 3-3: The addition of the
assertEquals
import statement is correctly implemented to support the updated assertion methods in theouterDeprecated
andinnerDeprecated
methods.- 14-14: Replacing
assert
statements withassertEquals
in theouterDeprecated
method is correctly done, aligning with the PR's goal of using TestNG's assertion methods for improved clarity and consistency.- 20-20: Similarly, replacing
assert
statements withassertEquals
in theinnerDeprecated
method is appropriate, enhancing the test's clarity and consistency with TestNG's assertion methods.testng-core/src/test/java/test/tmp/Tmp.java (1)
- 14-14: Modifying the
Thread.sleep
method call by replacingMath.abs(r.nextInt() % 300)
withMath.abs(r.nextInt(300))
is a valid improvement. This change simplifies the calculation of the sleep duration and avoids potential issues with the modulo operation. It's a good example of code modernization and simplification.testng-core/src/test/java/test/dataprovider/issue2565/SampleTestUsingPredicate.java (1)
- 14-14: Assigning the result of
data.test("IronHide")
to a boolean variableignored
is a valid change, but it's unclear if this modification serves a specific purpose related to the PR's objectives. It might be part of addressing a warning from Error Prone, but without context, it's hard to assess its relevance.Please clarify if this change is directly related to addressing an Error Prone warning or if it serves another purpose.
testng-core/src/test/java/test/dependent/SampleDependentConfigurationMethods.java (3)
- 3-3: The addition of the
assertTrue
import statement is correctly implemented to support the updated assertion methods in theSampleDependentConfigurationMethods
class.- 19-19: Replacing the
assert
statement withassertTrue
in thefirstInvocation
method is appropriate, enhancing the method's clarity and consistency with TestNG's assertion methods.- 25-26: Similarly, replacing
assert
statements withassertTrue
in theverifyDependents
method is correctly done, aligning with the PR's goal of using TestNG's assertion methods for improved clarity and consistency.testng-core/src/test/java/test/dataprovider/issue2327/TestClassSample.java (2)
- 4-4: Replacing the
LinkedList
import withArrayList
is a valid change that aligns with the PR's objective of code quality improvements, including the use of more efficient Java collections where appropriate.- 20-20: The replacement of
LinkedList
withArrayList
in thedp
method is correctly implemented, enhancing the code by utilizing a more suitable collection type for the given context.testng-core/src/test/java/test/dataprovider/issue2565/SampleTestUsingFunction.java (1)
- 15-15: Assigning the result of
data.apply("Bumble_Bee")
to aString
variableignored
is a valid change, but it's unclear if this modification serves a specific purpose related to the PR's objectives. It might be part of addressing a warning from Error Prone, but without context, it's hard to assess its relevance.Please clarify if this change is directly related to addressing an Error Prone warning or if it serves another purpose.
testng-core/src/test/java/test/NestedStaticTest.java (1)
- 21-21: Simplifying the initialization of a
Set
by replacing a verbose anonymous inner class withSet.of()
is a valid improvement that enhances readability and conciseness. This change aligns with the PR's objective of code modernization.testng-core/src/test/java/test/factory/FactoryInterleavingTest.java (2)
- 3-3: The addition of the
assertThat
import statement from AssertJ is correctly implemented to support the updated assertion method in themethodsShouldBeInterleaved
method.- 30-32: Replacing
Assert
withassertThat
from AssertJ for array comparison in themethodsShouldBeInterleaved
method is a valid improvement. This change enhances the readability and flexibility of the test, aligning with the PR's objective of enhancing code quality.testng-core/src/test/java/test/sample/Basic2.java (3)
- 3-4: The addition of static imports for
assertEquals
andassertTrue
fromorg.testng.Assert
is correctly implemented to support the updated assertion methods in theBasic2
class.- 17-17: Replacing
assert
statements withassertTrue
in thebasic2
method is appropriate, enhancing the test's clarity and consistency with TestNG's assertion methods.- 29-31: Similarly, replacing
assert
statements withassertTrue
andassertEquals
in thecheckTestAtClassLevelWasRun
method is correctly done, aligning with the PR's goal of using TestNG's assertion methods for improved clarity and consistency.testng-core/src/test/java/test/CtorCalledOnce.java (4)
- 3-3: The addition of the
assertEquals
import statement is correctly implemented to support the updated assertion methods in theCtorCalledOnce
class.- 21-21: Replacing
assert
statements withassertEquals
in thetestMethod1
method is correctly done, enhancing the test's clarity and consistency with TestNG's assertion methods.- 26-26: Similarly, replacing
assert
statements withassertEquals
in thetestMethod2
method is appropriate, aligning with the PR's goal of using TestNG's assertion methods for improved clarity and consistency.- 31-31: Again, replacing
assert
statements withassertEquals
in thetestMethod3
method is correctly implemented, enhancing the test's clarity and consistency with TestNG's assertion methods.testng-core/src/test/java/test/methods/SampleMethod1.java (2)
- 3-3: The addition of the
assertTrue
import statement is correctly implemented to support the updated assertion method in theverify
method.- 46-48: Replacing
assert
withassertTrue
in theverify
method is appropriate, enhancing the method's clarity and consistency with TestNG's assertion methods.testng-core/src/test/java/test/Exclude.java (2)
- 3-3: The addition of the
assertTrue
import statement is correctly implemented to support the updated assertion method in theverify
method.- 37-39: Refactoring the assertion in the
verify
method to useassertTrue
from TestNG is a valid improvement, enhancing the readability and maintainability of the code.testng-core/src/test/java/test/dependent/BaseOrderMethodTest.java (2)
- 3-3: The addition of the
assertTrue
import statement is correctly implemented to support the updated assertion method in theverifyGroup
method.- 35-37: Replacing
assert
withassertTrue
in theverifyGroup
method is appropriate, enhancing the method's clarity and consistency with TestNG's assertion methods.testng-core/src/test/java/test/ClassConfigurations.java (1)
- 34-35: The replacement of
assert
statements withassertEquals
intestOne
,testTwo
, andtestThree
methods improves test clarity and error messaging by providing more informative failure messages. This is a positive change.Also applies to: 40-41, 46-47
testng-core/src/test/java/test/factory/FactoryInSeparateClassTest.java (1)
- 39-43: Replacing the
assert
statement withassertEquals
in thecheckSum
method enhances test clarity and provides a more informative error message. This is a beneficial change for debugging and understanding test failures.testng-core/src/test/java/test/sample/BaseSampleInheritance.java (1)
- 43-44: The replacement of
assert
statements withassertTrue
in thetestBooleans
method improves test clarity and error messaging by providing more informative failure messages. This is a positive change.testng-core/src/main/java/org/testng/SuiteResult.java (1)
- 29-29: The modification of the
compareTo
method to compare instances ofSuiteResult
instead ofISuiteResult
could enhance the specificity of comparisons. However, it's important to verify the impact of this change on the overall sorting and comparison logic within the application.testng-core/src/test/java/test/dependent/SampleDependentMethods3.java (1)
- 23-23: Replacing
assert
statements with TestNG assertion methods likeassertFalse
andassertTrue
inSampleDependentMethods3.java
enhances test clarity and provides more informative error messages. This is a beneficial change for debugging and understanding test outcomes.Also applies to: 30-31, 37-39, 45-47
testng-core/src/test/java/test/MethodTest.java (1)
- 41-41: Replacing the
assert
statement withassertEquals
in theexcludePackage
method enhances test clarity and provides a more informative error message. This is a beneficial change for debugging and understanding test failures.testng-core/src/main/java/org/testng/internal/Tarjan.java (1)
- 17-17: Replacing the usage of
Stack
withArrayDeque
for thestack
field in theTarjan
class is a positive change, enhancing efficiency and modernizing the code. This adaptation aligns with best practices for stack operations in Java.Also applies to: 23-23
testng-core/src/test/java/test/junitreports/TestSuiteHandler.java (1)
- 13-14: Replacing the usage of
Stack
withDeque
forelementStack
andtestcaseStack
in theTestSuiteHandler
class is a positive change, enhancing efficiency and aligning with best practices for stack operations in Java. This adaptation improves safety and efficiency in handling elements and test cases during XML parsing.build-logic/jvm/src/main/kotlin/testng.java.gradle.kts (1)
- 9-9: Adding the
testng.errorprone
plugin to the Gradle build script is a positive change, enhancing code quality by integrating static analysis into the build process. This addition aligns with the project's goal of improving code reliability and catching common Java mistakes at compile-time.testng-core/src/test/java/test/dependent/SampleDependentMethods.java (1)
- 22-22: Replacing
assert
statements with TestNG assertion methods likeassertTrue
andassertFalse
inSampleDependentMethods.java
enhances test clarity and provides more informative error messages. This is a beneficial change for debugging and understanding test outcomes.Also applies to: 31-33, 39-42, 48-48, 54-57
testng-core/src/test/java/test/dependent/SampleDependentMethods2.java (1)
- 22-22: Replacing
assert
statements with TestNG assertion methods likeassertTrue
andassertFalse
inSampleDependentMethods2.java
enhances test clarity and provides more informative error messages. This is a beneficial change for debugging and understanding test outcomes.Also applies to: 31-33, 39-42, 48-48, 54-57
testng-core/src/test/java/test/custom/CustomAttributesTransformer.java (1)
- 48-52: Overriding the
equals
andhashCode
methods in theMoreAttribute
class to compare key and values arrays is a good practice, ensuring correct behavior in collections or comparisons. The use ofArrays.equals
andObjects.hash
aligns with Java best practices.Also applies to: 56-59
testng-core/src/test/java/test/sample/AfterClassCalledAtEnd.java (1)
- 28-30: Replacing
assert
statements withassertTrue
andassertFalse
in theafterClass
,test1
,test2
, andtest3
methods enhances test clarity and readability by explicitly stating the conditions being checked. This is a positive change.Also applies to: 36-37, 43-44, 50-51
testng-core/src/test/java/test/SampleInheritance.java (1)
- 23-25: Replacing
assert
statements withassertEquals
andassertTrue
in the test methods ofSampleInheritance.java
ensures more informative failure messages and improved test clarity. This is a positive change.Also applies to: 33-34, 39-42
testng-core/src/test/java/test/junitreports/Testsuite.java (1)
- 95-95: Switching from
LinkedList
toArrayList
for thetestcase
field in theTestsuite
class can improve performance and reduce memory usage, especially if the list is primarily used for random access operations. This is a positive change.build-logic/build-parameters/build.gradle.kts (1)
- 52-55: The addition of the
skipErrorProne
parameter with a default value offalse
is correctly implemented and aligns with best practices for enhancing code quality through static analysis. Good job on making Error Prone checks enabled by default and providing a clear description.testng-core/src/main/java/org/testng/internal/ClonedMethod.java (1)
- 141-141: The modification to return the declaring class of
m_javaMethod
in thegetRealClass()
method is a logical and correct improvement. This change ensures accurate identification of the class where the method is defined, especially important in inheritance scenarios.testng-core/src/main/java/org/testng/internal/Graph.java (1)
- 192-200: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [195-211]
Replacing
LinkedList
withArrayDeque
forresult
andqueue
in thefindPredecessors
method, and changing the return type toArrayList
, are performance improvements and align with best practices for using Java collections. These changes enhance efficiency, especially for queue operations, and are more suitable for scenarios where random access or efficient add/remove operations are important.testng-runner-api/src/main/java/org/testng/internal/LiteWeightTestNGMethod.java (1)
- 30-30: The change from
LinkedList
toArrayList
for initializingmethodsDependedUpon
is a good practice for most use cases, asArrayList
typically offers better performance for random access operations. Ensure that all operations performed onmethodsDependedUpon
are compatible withArrayList
.testng-core/src/test/java/test/junitreports/JUnitReportsTest.java (1)
- 108-108: Replacing
LinkedList
withList.of
for initializing a list of strings is a modern and concise approach, enhancing code readability. Just ensure that immutability of the list created byList.of
is acceptable in this context, as it cannot be modified after creation.testng-asserts/src/test/java/org/testng/AssertTest.java (3)
- 310-311: The refactoring to use
Map.of
for initializingmapActual
andmapExpected
in theassertEqualsMapShouldFail
method is a good improvement for readability and conciseness. This change aligns with modern Java practices and makes the code cleaner.- 646-648: Adding the
@Override
annotation to thehashCode
method in theBrokenEqualsTrue
class is a good practice. It clearly indicates that this method is overriding a method from its superclass, enhancing readability and maintainability of the code.- 658-660: Similarly, adding the
@Override
annotation to thehashCode
method in theBrokenEqualsFalse
class improves code clarity and maintainability by explicitly indicating the method's purpose. This is a positive change.testng-core/src/main/java/org/testng/internal/annotations/JDK15TagFactory.java (1)
- 647-647: The change to use
annotationType().getMethod(methodName)
instead ofgetClass().getMethod(methodName)
is correct and improves the method lookup behavior for annotations. This approach ensures that the specific method of the annotation type is retrieved, which is essential for the correct functioning of theinvokeMethod
utility.However, it might be beneficial to add a comment explaining why
annotationType()
is used instead ofgetClass()
for future maintainability and clarity for other developers who might work on this code.testng-core/src/test/java/test/thread/parallelization/BaseParallelizationTest.java (1)
- 781-781: The change from
a.getClass()
toa.annotationType()
for checking if an annotation is assignable fromorg.testng.annotations.Test.class
is a significant improvement. This adjustment ensures that the code correctly identifies annotations on methods, asannotationType()
accurately returns the annotation's type, making theisAssignableFrom
check valid and precise. Good job on making this change to enhance the correctness of the annotation processing logic.
@vlsi - It would be good if you could please help take a quick look and add your feedback. Once done, I will go ahead and get this merged. |
@@ -6,6 +6,7 @@ plugins { | |||
id("testng.versioning") | |||
id("testng.style") | |||
id("testng.repositories") | |||
id("testng.errorprone") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably be better to have something like if (buildParameters.skipErrorProne) { apply(plugin = "testng.errorprone")
below.
Then errorprone
won't be attached to the project if skipErrorProne
activated.
} | ||
|
||
tasks.withType<JavaCompile>().configureEach { | ||
options.errorprone.isEnabled = !buildParameters.skipErrorProne |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A slightly better option would be avoid verifying buildParameters.skipErrorProne
and go with conditional apply of testng.errorprone
plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an example: https://github.com/pgjdbc/pgjdbc/blob/93b0fcb2711d9c1e3a2a03134369738a02a58b40/build-logic/verification/src/main/kotlin/build-logic.style.gradle.kts#L44-L46
However, both ways work.
The slight difference is as follows: imagine there's an issue with errorprone plugin. For instance, imagine errorprone fails with NPE. If you completely skip net.ltgt.errorprone
then it would likely allow to avoid failures. Of course, errorprone.isEnabled
might work, however, it might fail to work :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your feedback.
I will update
ac5ead9
to
a0adc45
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (52)
- build-logic/build-parameters/build.gradle.kts (1 hunks)
- build-logic/code-quality/build.gradle.kts (1 hunks)
- build-logic/code-quality/src/main/kotlin/testng.errorprone.gradle.kts (1 hunks)
- build-logic/jvm/src/main/kotlin/testng.java.gradle.kts (1 hunks)
- testng-asserts/src/test/java/org/testng/AssertTest.java (2 hunks)
- testng-core/src/main/java/org/testng/SuiteResult.java (2 hunks)
- testng-core/src/main/java/org/testng/internal/ClonedMethod.java (1 hunks)
- testng-core/src/main/java/org/testng/internal/Graph.java (3 hunks)
- testng-core/src/main/java/org/testng/internal/Tarjan.java (2 hunks)
- testng-core/src/main/java/org/testng/internal/annotations/JDK15TagFactory.java (1 hunks)
- testng-core/src/test/java/NoPackageTest.java (2 hunks)
- testng-core/src/test/java/test/ClassConfigurations.java (2 hunks)
- testng-core/src/test/java/test/CtorCalledOnce.java (2 hunks)
- testng-core/src/test/java/test/Exclude.java (2 hunks)
- testng-core/src/test/java/test/MethodTest.java (2 hunks)
- testng-core/src/test/java/test/NestedStaticTest.java (2 hunks)
- testng-core/src/test/java/test/SampleInheritance.java (2 hunks)
- testng-core/src/test/java/test/classgroup/Second.java (1 hunks)
- testng-core/src/test/java/test/configuration/issue2729/BeforeConfigTestSample.java (1 hunks)
- testng-core/src/test/java/test/custom/CustomAttributesTransformer.java (2 hunks)
- testng-core/src/test/java/test/dataprovider/issue2327/TestClassSample.java (2 hunks)
- testng-core/src/test/java/test/dataprovider/issue2565/SampleTestUsingFunction.java (1 hunks)
- testng-core/src/test/java/test/dataprovider/issue2565/SampleTestUsingPredicate.java (1 hunks)
- testng-core/src/test/java/test/dependent/BaseOrderMethodTest.java (2 hunks)
- testng-core/src/test/java/test/dependent/SampleDependentConfigurationMethods.java (2 hunks)
- testng-core/src/test/java/test/dependent/SampleDependentMethods.java (3 hunks)
- testng-core/src/test/java/test/dependent/SampleDependentMethods2.java (3 hunks)
- testng-core/src/test/java/test/dependent/SampleDependentMethods3.java (2 hunks)
- testng-core/src/test/java/test/factory/FactoryInSeparateClassTest.java (2 hunks)
- testng-core/src/test/java/test/factory/FactoryInterleavingTest.java (2 hunks)
- testng-core/src/test/java/test/hook/issue2251/SampleTestCase.java (1 hunks)
- testng-core/src/test/java/test/junitreports/JUnitReportsTest.java (2 hunks)
- testng-core/src/test/java/test/junitreports/TestSuiteHandler.java (1 hunks)
- testng-core/src/test/java/test/junitreports/Testsuite.java (2 hunks)
- testng-core/src/test/java/test/methods/SampleMethod1.java (2 hunks)
- testng-core/src/test/java/test/objectfactory/ContextAwareObjectFactoryFactory.java (2 hunks)
- testng-core/src/test/java/test/priority/parallel/EfficientPriorityParallelizationTest2.java (1 hunks)
- testng-core/src/test/java/test/sample/AfterClassCalledAtEnd.java (2 hunks)
- testng-core/src/test/java/test/sample/BaseAfterClassCalledAtEnd.java (1 hunks)
- testng-core/src/test/java/test/sample/BaseSampleInheritance.java (2 hunks)
- testng-core/src/test/java/test/sample/Basic1.java (2 hunks)
- testng-core/src/test/java/test/sample/Basic2.java (3 hunks)
- testng-core/src/test/java/test/sample/InvocationCountTest.java (2 hunks)
- testng-core/src/test/java/test/sample/JUnitSample2.java (1 hunks)
- testng-core/src/test/java/test/sample/PartialGroupVerification.java (2 hunks)
- testng-core/src/test/java/test/sample/Scope.java (2 hunks)
- testng-core/src/test/java/test/superclass/BaseSampleTest3.java (1 hunks)
- testng-core/src/test/java/test/thread/parallelization/BaseParallelizationTest.java (1 hunks)
- testng-core/src/test/java/test/tmp/Tmp.java (1 hunks)
- testng-core/src/test/java/test/triangle/Child1.java (2 hunks)
- testng-core/src/test/java/test/triangle/Child2.java (2 hunks)
- testng-runner-api/src/main/java/org/testng/internal/LiteWeightTestNGMethod.java (2 hunks)
Files skipped from review as they are similar to previous changes (52)
- build-logic/build-parameters/build.gradle.kts
- build-logic/code-quality/build.gradle.kts
- build-logic/code-quality/src/main/kotlin/testng.errorprone.gradle.kts
- build-logic/jvm/src/main/kotlin/testng.java.gradle.kts
- testng-asserts/src/test/java/org/testng/AssertTest.java
- testng-core/src/main/java/org/testng/SuiteResult.java
- testng-core/src/main/java/org/testng/internal/ClonedMethod.java
- testng-core/src/main/java/org/testng/internal/Graph.java
- testng-core/src/main/java/org/testng/internal/Tarjan.java
- testng-core/src/main/java/org/testng/internal/annotations/JDK15TagFactory.java
- testng-core/src/test/java/NoPackageTest.java
- testng-core/src/test/java/test/ClassConfigurations.java
- testng-core/src/test/java/test/CtorCalledOnce.java
- testng-core/src/test/java/test/Exclude.java
- testng-core/src/test/java/test/MethodTest.java
- testng-core/src/test/java/test/NestedStaticTest.java
- testng-core/src/test/java/test/SampleInheritance.java
- testng-core/src/test/java/test/classgroup/Second.java
- testng-core/src/test/java/test/configuration/issue2729/BeforeConfigTestSample.java
- testng-core/src/test/java/test/custom/CustomAttributesTransformer.java
- testng-core/src/test/java/test/dataprovider/issue2327/TestClassSample.java
- testng-core/src/test/java/test/dataprovider/issue2565/SampleTestUsingFunction.java
- testng-core/src/test/java/test/dataprovider/issue2565/SampleTestUsingPredicate.java
- testng-core/src/test/java/test/dependent/BaseOrderMethodTest.java
- testng-core/src/test/java/test/dependent/SampleDependentConfigurationMethods.java
- testng-core/src/test/java/test/dependent/SampleDependentMethods.java
- testng-core/src/test/java/test/dependent/SampleDependentMethods2.java
- testng-core/src/test/java/test/dependent/SampleDependentMethods3.java
- testng-core/src/test/java/test/factory/FactoryInSeparateClassTest.java
- testng-core/src/test/java/test/factory/FactoryInterleavingTest.java
- testng-core/src/test/java/test/hook/issue2251/SampleTestCase.java
- testng-core/src/test/java/test/junitreports/JUnitReportsTest.java
- testng-core/src/test/java/test/junitreports/TestSuiteHandler.java
- testng-core/src/test/java/test/junitreports/Testsuite.java
- testng-core/src/test/java/test/methods/SampleMethod1.java
- testng-core/src/test/java/test/objectfactory/ContextAwareObjectFactoryFactory.java
- testng-core/src/test/java/test/priority/parallel/EfficientPriorityParallelizationTest2.java
- testng-core/src/test/java/test/sample/AfterClassCalledAtEnd.java
- testng-core/src/test/java/test/sample/BaseAfterClassCalledAtEnd.java
- testng-core/src/test/java/test/sample/BaseSampleInheritance.java
- testng-core/src/test/java/test/sample/Basic1.java
- testng-core/src/test/java/test/sample/Basic2.java
- testng-core/src/test/java/test/sample/InvocationCountTest.java
- testng-core/src/test/java/test/sample/JUnitSample2.java
- testng-core/src/test/java/test/sample/PartialGroupVerification.java
- testng-core/src/test/java/test/sample/Scope.java
- testng-core/src/test/java/test/superclass/BaseSampleTest3.java
- testng-core/src/test/java/test/thread/parallelization/BaseParallelizationTest.java
- testng-core/src/test/java/test/tmp/Tmp.java
- testng-core/src/test/java/test/triangle/Child1.java
- testng-core/src/test/java/test/triangle/Child2.java
- testng-runner-api/src/main/java/org/testng/internal/LiteWeightTestNGMethod.java
a0adc45
to
6c97402
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (52)
- build-logic/build-parameters/build.gradle.kts (1 hunks)
- build-logic/code-quality/build.gradle.kts (1 hunks)
- build-logic/code-quality/src/main/kotlin/testng.errorprone.gradle.kts (1 hunks)
- build-logic/jvm/src/main/kotlin/testng.java.gradle.kts (1 hunks)
- testng-asserts/src/test/java/org/testng/AssertTest.java (2 hunks)
- testng-core/src/main/java/org/testng/SuiteResult.java (2 hunks)
- testng-core/src/main/java/org/testng/internal/ClonedMethod.java (1 hunks)
- testng-core/src/main/java/org/testng/internal/Graph.java (3 hunks)
- testng-core/src/main/java/org/testng/internal/Tarjan.java (2 hunks)
- testng-core/src/main/java/org/testng/internal/annotations/JDK15TagFactory.java (1 hunks)
- testng-core/src/test/java/NoPackageTest.java (2 hunks)
- testng-core/src/test/java/test/ClassConfigurations.java (2 hunks)
- testng-core/src/test/java/test/CtorCalledOnce.java (2 hunks)
- testng-core/src/test/java/test/Exclude.java (2 hunks)
- testng-core/src/test/java/test/MethodTest.java (2 hunks)
- testng-core/src/test/java/test/NestedStaticTest.java (2 hunks)
- testng-core/src/test/java/test/SampleInheritance.java (2 hunks)
- testng-core/src/test/java/test/classgroup/Second.java (1 hunks)
- testng-core/src/test/java/test/configuration/issue2729/BeforeConfigTestSample.java (1 hunks)
- testng-core/src/test/java/test/custom/CustomAttributesTransformer.java (2 hunks)
- testng-core/src/test/java/test/dataprovider/issue2327/TestClassSample.java (2 hunks)
- testng-core/src/test/java/test/dataprovider/issue2565/SampleTestUsingFunction.java (1 hunks)
- testng-core/src/test/java/test/dataprovider/issue2565/SampleTestUsingPredicate.java (1 hunks)
- testng-core/src/test/java/test/dependent/BaseOrderMethodTest.java (2 hunks)
- testng-core/src/test/java/test/dependent/SampleDependentConfigurationMethods.java (2 hunks)
- testng-core/src/test/java/test/dependent/SampleDependentMethods.java (3 hunks)
- testng-core/src/test/java/test/dependent/SampleDependentMethods2.java (3 hunks)
- testng-core/src/test/java/test/dependent/SampleDependentMethods3.java (2 hunks)
- testng-core/src/test/java/test/factory/FactoryInSeparateClassTest.java (2 hunks)
- testng-core/src/test/java/test/factory/FactoryInterleavingTest.java (2 hunks)
- testng-core/src/test/java/test/hook/issue2251/SampleTestCase.java (1 hunks)
- testng-core/src/test/java/test/junitreports/JUnitReportsTest.java (2 hunks)
- testng-core/src/test/java/test/junitreports/TestSuiteHandler.java (1 hunks)
- testng-core/src/test/java/test/junitreports/Testsuite.java (2 hunks)
- testng-core/src/test/java/test/methods/SampleMethod1.java (2 hunks)
- testng-core/src/test/java/test/objectfactory/ContextAwareObjectFactoryFactory.java (2 hunks)
- testng-core/src/test/java/test/priority/parallel/EfficientPriorityParallelizationTest2.java (1 hunks)
- testng-core/src/test/java/test/sample/AfterClassCalledAtEnd.java (2 hunks)
- testng-core/src/test/java/test/sample/BaseAfterClassCalledAtEnd.java (1 hunks)
- testng-core/src/test/java/test/sample/BaseSampleInheritance.java (2 hunks)
- testng-core/src/test/java/test/sample/Basic1.java (2 hunks)
- testng-core/src/test/java/test/sample/Basic2.java (3 hunks)
- testng-core/src/test/java/test/sample/InvocationCountTest.java (2 hunks)
- testng-core/src/test/java/test/sample/JUnitSample2.java (1 hunks)
- testng-core/src/test/java/test/sample/PartialGroupVerification.java (2 hunks)
- testng-core/src/test/java/test/sample/Scope.java (2 hunks)
- testng-core/src/test/java/test/superclass/BaseSampleTest3.java (1 hunks)
- testng-core/src/test/java/test/thread/parallelization/BaseParallelizationTest.java (1 hunks)
- testng-core/src/test/java/test/tmp/Tmp.java (1 hunks)
- testng-core/src/test/java/test/triangle/Child1.java (2 hunks)
- testng-core/src/test/java/test/triangle/Child2.java (2 hunks)
- testng-runner-api/src/main/java/org/testng/internal/LiteWeightTestNGMethod.java (2 hunks)
Files skipped from review as they are similar to previous changes (52)
- build-logic/build-parameters/build.gradle.kts
- build-logic/code-quality/build.gradle.kts
- build-logic/code-quality/src/main/kotlin/testng.errorprone.gradle.kts
- build-logic/jvm/src/main/kotlin/testng.java.gradle.kts
- testng-asserts/src/test/java/org/testng/AssertTest.java
- testng-core/src/main/java/org/testng/SuiteResult.java
- testng-core/src/main/java/org/testng/internal/ClonedMethod.java
- testng-core/src/main/java/org/testng/internal/Graph.java
- testng-core/src/main/java/org/testng/internal/Tarjan.java
- testng-core/src/main/java/org/testng/internal/annotations/JDK15TagFactory.java
- testng-core/src/test/java/NoPackageTest.java
- testng-core/src/test/java/test/ClassConfigurations.java
- testng-core/src/test/java/test/CtorCalledOnce.java
- testng-core/src/test/java/test/Exclude.java
- testng-core/src/test/java/test/MethodTest.java
- testng-core/src/test/java/test/NestedStaticTest.java
- testng-core/src/test/java/test/SampleInheritance.java
- testng-core/src/test/java/test/classgroup/Second.java
- testng-core/src/test/java/test/configuration/issue2729/BeforeConfigTestSample.java
- testng-core/src/test/java/test/custom/CustomAttributesTransformer.java
- testng-core/src/test/java/test/dataprovider/issue2327/TestClassSample.java
- testng-core/src/test/java/test/dataprovider/issue2565/SampleTestUsingFunction.java
- testng-core/src/test/java/test/dataprovider/issue2565/SampleTestUsingPredicate.java
- testng-core/src/test/java/test/dependent/BaseOrderMethodTest.java
- testng-core/src/test/java/test/dependent/SampleDependentConfigurationMethods.java
- testng-core/src/test/java/test/dependent/SampleDependentMethods.java
- testng-core/src/test/java/test/dependent/SampleDependentMethods2.java
- testng-core/src/test/java/test/dependent/SampleDependentMethods3.java
- testng-core/src/test/java/test/factory/FactoryInSeparateClassTest.java
- testng-core/src/test/java/test/factory/FactoryInterleavingTest.java
- testng-core/src/test/java/test/hook/issue2251/SampleTestCase.java
- testng-core/src/test/java/test/junitreports/JUnitReportsTest.java
- testng-core/src/test/java/test/junitreports/TestSuiteHandler.java
- testng-core/src/test/java/test/junitreports/Testsuite.java
- testng-core/src/test/java/test/methods/SampleMethod1.java
- testng-core/src/test/java/test/objectfactory/ContextAwareObjectFactoryFactory.java
- testng-core/src/test/java/test/priority/parallel/EfficientPriorityParallelizationTest2.java
- testng-core/src/test/java/test/sample/AfterClassCalledAtEnd.java
- testng-core/src/test/java/test/sample/BaseAfterClassCalledAtEnd.java
- testng-core/src/test/java/test/sample/BaseSampleInheritance.java
- testng-core/src/test/java/test/sample/Basic1.java
- testng-core/src/test/java/test/sample/Basic2.java
- testng-core/src/test/java/test/sample/InvocationCountTest.java
- testng-core/src/test/java/test/sample/JUnitSample2.java
- testng-core/src/test/java/test/sample/PartialGroupVerification.java
- testng-core/src/test/java/test/sample/Scope.java
- testng-core/src/test/java/test/superclass/BaseSampleTest3.java
- testng-core/src/test/java/test/thread/parallelization/BaseParallelizationTest.java
- testng-core/src/test/java/test/tmp/Tmp.java
- testng-core/src/test/java/test/triangle/Child1.java
- testng-core/src/test/java/test/triangle/Child2.java
- testng-runner-api/src/main/java/org/testng/internal/LiteWeightTestNGMethod.java
Related to #2941
Summary by CodeRabbit
New Features
Refactor
assert
statements with TestNG's assertion methods (assertTrue
,assertFalse
,assertEquals
).LinkedList
andStack
with more efficient alternatives likeArrayList
andArrayDeque
.Bug Fixes