Add errorprone #2958
Merged
Add errorprone #2958
Conversation
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
|
@krmahadevan As suggested by @vlsi , I added errorprone only in a single modification. |
vlsi
reviewed
Aug 15, 2023
| suiteOneTestOneTestMethodLevelEventLogs.stream() | ||
| .filter(e -> e.getEvent() == TestNgRunEvent.LISTENER_TEST_METHOD_START); |
There was a problem hiding this comment.
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.
@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.
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.
Yes. I will consider it when it will be the last failing test :)
There was a problem hiding this comment.
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.
juherr
force-pushed
the
feature/errorprone
branch
from
490010d to
14cb1ad
Compare
juherr
force-pushed
the
feature/errorprone
branch
from
14cb1ad to
2632ff5
Compare
juherr
force-pushed
the
feature/errorprone
branch
from
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. |
juherr
force-pushed
the
feature/errorprone
branch
3 times, most recently
from
1915c56 to
23371c7
Compare
|
@krmahadevan There is still many warnings but we can merge once you'll reviewed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
assertTrueimport 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
assertTrueis correctly added to support the updated assertion method in the test case.- 16-16: The replacement of the
assertstatement withassertTrueis 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
assertTrueimport statement is correctly implemented to support the updated assertion method in the test case.- 12-12: Replacing the
assertstatement withassertTruecorrectly 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-plugindependency 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
assertTrueimport statement is correctly implemented to support the updated assertion method in thebaseAfterClassmethod.- 12-12: Replacing the
assertstatement withassertTruein thebaseAfterClassmethod 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
assertTrueimport statement is correctly implemented to support the updated assertion method in the test case.- 15-17: Replacing the
assertstatement withassertTruein theverifymethod 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
assertTrueimport statement is correctly implemented to support the updated assertion method in theaftermethod.- 17-17: Replacing the
assertstatement withassertTruein theaftermethod 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 aStringvariableunusedis 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
errorpronedependency 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
assertTrueimport statement is correctly implemented to support the updated assertion methods in thechild1andchild1amethods.- 15-15: Replacing the
assertstatement withassertTruein thechild1method is appropriate, enhancing the test's clarity and consistency with TestNG's assertion methods.- 20-20: Similarly, replacing the
assertstatement withassertTruein thechild1amethod 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
assertNotNullimport statement is correctly implemented to support the updated assertion method in thecreatemethod.- 16-16: Replacing the
assertstatement withassertNotNullin thecreatemethod 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
assertNotNullin thetestSample2ThatSetUpWasRunmethod 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
assertTrueimport statement is correctly implemented to support the updated assertion method in thebasic1method.- 26-26: Replacing the
assertstatement withassertTruein thebasic1method 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
assertEqualsimport statement is correctly implemented to support the updated assertion methods in theouterDeprecatedandinnerDeprecatedmethods.- 14-14: Replacing
assertstatements withassertEqualsin theouterDeprecatedmethod is correctly done, aligning with the PR's goal of using TestNG's assertion methods for improved clarity and consistency.- 20-20: Similarly, replacing
assertstatements withassertEqualsin theinnerDeprecatedmethod 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.sleepmethod 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 variableignoredis 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
assertTrueimport statement is correctly implemented to support the updated assertion methods in theSampleDependentConfigurationMethodsclass.- 19-19: Replacing the
assertstatement withassertTruein thefirstInvocationmethod is appropriate, enhancing the method's clarity and consistency with TestNG's assertion methods.- 25-26: Similarly, replacing
assertstatements withassertTruein theverifyDependentsmethod 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
LinkedListimport withArrayListis 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
LinkedListwithArrayListin thedpmethod 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 aStringvariableignoredis 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
Setby 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
assertThatimport statement from AssertJ is correctly implemented to support the updated assertion method in themethodsShouldBeInterleavedmethod.- 30-32: Replacing
AssertwithassertThatfrom AssertJ for array comparison in themethodsShouldBeInterleavedmethod 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
assertEqualsandassertTruefromorg.testng.Assertis correctly implemented to support the updated assertion methods in theBasic2class.- 17-17: Replacing
assertstatements withassertTruein thebasic2method is appropriate, enhancing the test's clarity and consistency with TestNG's assertion methods.- 29-31: Similarly, replacing
assertstatements withassertTrueandassertEqualsin thecheckTestAtClassLevelWasRunmethod 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
assertEqualsimport statement is correctly implemented to support the updated assertion methods in theCtorCalledOnceclass.- 21-21: Replacing
assertstatements withassertEqualsin thetestMethod1method is correctly done, enhancing the test's clarity and consistency with TestNG's assertion methods.- 26-26: Similarly, replacing
assertstatements withassertEqualsin thetestMethod2method is appropriate, aligning with the PR's goal of using TestNG's assertion methods for improved clarity and consistency.- 31-31: Again, replacing
assertstatements withassertEqualsin thetestMethod3method 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
assertTrueimport statement is correctly implemented to support the updated assertion method in theverifymethod.- 46-48: Replacing
assertwithassertTruein theverifymethod 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
assertTrueimport statement is correctly implemented to support the updated assertion method in theverifymethod.- 37-39: Refactoring the assertion in the
verifymethod to useassertTruefrom 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
assertTrueimport statement is correctly implemented to support the updated assertion method in theverifyGroupmethod.- 35-37: Replacing
assertwithassertTruein theverifyGroupmethod 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
assertstatements withassertEqualsintestOne,testTwo, andtestThreemethods 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
assertstatement withassertEqualsin thecheckSummethod 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
assertstatements withassertTruein thetestBooleansmethod 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
compareTomethod to compare instances ofSuiteResultinstead ofISuiteResultcould 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
assertstatements with TestNG assertion methods likeassertFalseandassertTrueinSampleDependentMethods3.javaenhances 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
assertstatement withassertEqualsin theexcludePackagemethod 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
StackwithArrayDequefor thestackfield in theTarjanclass 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
StackwithDequeforelementStackandtestcaseStackin theTestSuiteHandlerclass 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.errorproneplugin 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
assertstatements with TestNG assertion methods likeassertTrueandassertFalseinSampleDependentMethods.javaenhances 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
assertstatements with TestNG assertion methods likeassertTrueandassertFalseinSampleDependentMethods2.javaenhances 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
equalsandhashCodemethods in theMoreAttributeclass to compare key and values arrays is a good practice, ensuring correct behavior in collections or comparisons. The use ofArrays.equalsandObjects.hashaligns with Java best practices.Also applies to: 56-59
testng-core/src/test/java/test/sample/AfterClassCalledAtEnd.java (1)
- 28-30: Replacing
assertstatements withassertTrueandassertFalsein theafterClass,test1,test2, andtest3methods 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
assertstatements withassertEqualsandassertTruein the test methods ofSampleInheritance.javaensures 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
LinkedListtoArrayListfor thetestcasefield in theTestsuiteclass 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
skipErrorProneparameter with a default value offalseis 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_javaMethodin 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
LinkedListwithArrayDequeforresultandqueuein thefindPredecessorsmethod, 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
LinkedListtoArrayListfor initializingmethodsDependedUponis a good practice for most use cases, asArrayListtypically offers better performance for random access operations. Ensure that all operations performed onmethodsDependedUponare compatible withArrayList.testng-core/src/test/java/test/junitreports/JUnitReportsTest.java (1)
- 108-108: Replacing
LinkedListwithList.offor initializing a list of strings is a modern and concise approach, enhancing code readability. Just ensure that immutability of the list created byList.ofis 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.offor initializingmapActualandmapExpectedin theassertEqualsMapShouldFailmethod is a good improvement for readability and conciseness. This change aligns with modern Java practices and makes the code cleaner.- 646-648: Adding the
@Overrideannotation to thehashCodemethod in theBrokenEqualsTrueclass 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
@Overrideannotation to thehashCodemethod in theBrokenEqualsFalseclass 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 theinvokeMethodutility.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.classis a significant improvement. This adjustment ensures that the code correctly identifies annotations on methods, asannotationType()accurately returns the annotation's type, making theisAssignableFromcheck valid and precise. Good job on making this change to enhance the correctness of the annotation processing logic.
krmahadevan
approved these changes
Feb 19, 2024
|
@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. |
vlsi
reviewed
Feb 19, 2024
| id("testng.versioning") | ||
| id("testng.style") | ||
| id("testng.repositories") | ||
| id("testng.errorprone") |
There was a problem hiding this comment.
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.
vlsi
reviewed
Feb 19, 2024
| } | ||
|
|
||
| tasks.withType<JavaCompile>().configureEach { | ||
| options.errorprone.isEnabled = !buildParameters.skipErrorProne |
There was a problem hiding this comment.
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.
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.
Thanks for your feedback.
I will update
juherr
force-pushed
the
feature/errorprone
branch
from
ac5ead9 to
a0adc45
Compare
There was a problem hiding this comment.
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
juherr
force-pushed
the
feature/errorprone
branch
from
a0adc45 to
6c97402
Compare
There was a problem hiding this comment.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Related to #2941
Summary by CodeRabbit
New Features
Refactor
assertstatements with TestNG's assertion methods (assertTrue,assertFalse,assertEquals).LinkedListandStackwith more efficient alternatives likeArrayListandArrayDeque.Bug Fixes