Streamline data provider listener invocation #3058
Conversation
WalkthroughThe recent updates focus on addressing issues related to Changes
Assessment against linked issues
Poem
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 (
|
![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: 2
Configuration used: CodeRabbit UI
Files selected for processing (6)
- CHANGES.txt (1 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java (2 hunks)
- testng-core/src/test/java/test/dataprovider/DataProviderTest.java (3 hunks)
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderListener.java (1 hunks)
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderTestClassSample.java (1 hunks)
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderWithoutListenerTestClassSample.java (1 hunks)
Additional comments: 11
testng-core/src/test/java/test/dataprovider/issue3045/DataProviderWithoutListenerTestClassSample.java (1)
- 1-18: LGTM!
testng-core/src/test/java/test/dataprovider/issue3045/DataProviderTestClassSample.java (1)
- 1-20: LGTM!
testng-core/src/test/java/test/dataprovider/issue3045/DataProviderListener.java (1)
- 1-35: LGTM!
testng-core/src/test/java/test/dataprovider/DataProviderTest.java (3)
- 25-26: LGTM!
- 59-61: LGTM!
- 674-702: LGTM!
testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java (4)
- 21-21: Import
java.util.function.BinaryOperatoris correctly added to support merging operations in thedataProviderListeners()method.- 23-23: Import
java.util.function.Functionis correctly added to support functional programming practices, specifically in thedataProviderListeners()method for mapping operations.- 270-272: The
className(IDataProviderListener listener)method is correctly implemented to return the class name of a listener. This utility method aids in identifying unique listeners by their class names, which is essential for the deduplication logic indataProviderListeners().- 284-286: The filtering logic in
dataProviderListeners()correctly ensures that only unique listeners are added from the suite level, preventing duplicate invocations. This implementation aligns with the PR's objective to invokeIDataProviderListenermethods only once per data provider method.CHANGES.txt (1)
- 3-3: The version number
7.10.0is mentioned without any associated changes. Please verify if this is intentional or if the changes for this version are missing.
| @@ -1,5 +1,6 @@ | |||
| Current | |||
| 7.10.0 | |||
| Fixed: GITHUB-3045: IDataProviderListener - beforeDataProviderExecution and afterDataProviderExecution are called twice in special setup (Krishnan Mahadevan) | |||
There was a problem hiding this comment.
The version 7.10.0 is listed without any subsequent changes or enhancements detailed, which deviates from the expected format of documenting version updates.
- 7.10.0
+ 7.10.0 (Date or detailed changes missing)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| Fixed: GITHUB-3045: IDataProviderListener - beforeDataProviderExecution and afterDataProviderExecution are called twice in special setup (Krishnan Mahadevan) | |
| Fixed: GITHUB-3045: IDataProviderListener - beforeDataProviderExecution and afterDataProviderExecution are called twice in special setup (Krishnan Mahadevan) | |
| 7.10.0 (Date or detailed changes missing) |
| BinaryOperator<IDataProviderListener> merger = (l1, l2) -> l1; | ||
| Map<String, IDataProviderListener> data = | ||
| this.holder.getListeners().stream() | ||
| .collect(Collectors.toMap(TestInvoker::className, Function.identity(), merger)); | ||
| Collection<IDataProviderListener> dpListeners = Sets.newHashSet(data.values()); |
There was a problem hiding this comment.
The use of BinaryOperator<IDataProviderListener> merger = (l1, l2) -> l1; in dataProviderListeners() effectively ensures that in case of duplicates, the first encountered listener is retained. This logic is crucial for preventing multiple invocations of the same listener. However, consider adding a comment explaining the choice of keeping the first listener in case of duplicates for future maintainability.
+ // In case of duplicate listeners, retain the first encountered instance
BinaryOperator<IDataProviderListener> merger = (l1, l2) -> l1;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| BinaryOperator<IDataProviderListener> merger = (l1, l2) -> l1; | |
| Map<String, IDataProviderListener> data = | |
| this.holder.getListeners().stream() | |
| .collect(Collectors.toMap(TestInvoker::className, Function.identity(), merger)); | |
| Collection<IDataProviderListener> dpListeners = Sets.newHashSet(data.values()); | |
| // In case of duplicate listeners, retain the first encountered instance | |
| BinaryOperator<IDataProviderListener> merger = (l1, l2) -> l1; | |
| Map<String, IDataProviderListener> data = | |
| this.holder.getListeners().stream() | |
| .collect(Collectors.toMap(TestInvoker::className, Function.identity(), merger)); | |
| Collection<IDataProviderListener> dpListeners = Sets.newHashSet(data.values()); |
I think the duplication happens when merging the listeners. That is why I am pruning the duplicates while iterating |
krmahadevan
force-pushed
the
bugfix/3045
branch
from
8770d6a
to
e5c2b4e
Compare
|
@juherr - please ignore my earlier comment. You were right. I refactored the code to add the pruning at the level wherein entries are being added. Please check now. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- CHANGES.txt (1 hunks)
- testng-core/src/main/java/org/testng/DataProviderHolder.java (4 hunks)
- testng-core/src/test/java/test/dataprovider/DataProviderTest.java (3 hunks)
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderListener.java (1 hunks)
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderTestClassSample.java (1 hunks)
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderWithoutListenerTestClassSample.java (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- CHANGES.txt
- testng-core/src/test/java/test/dataprovider/DataProviderTest.java
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderListener.java
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderTestClassSample.java
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderWithoutListenerTestClassSample.java
Additional comments: 4
testng-core/src/main/java/org/testng/DataProviderHolder.java (4)
- 15-15: Switching
listenersfrom aSetto aConcurrentMapis a good approach for ensuring thread safety and uniqueness of listeners based on their class. This change aligns with the PR's objective to prevent duplicate listener invocations.- 19-19: Returning an unmodifiable collection of the map's values in
getListenersmaintains immutability and thread safety of the listeners collection when exposed to external callers.- 31-31: Using
putIfAbsentinaddListenereffectively prevents duplicate listeners based on their class, aligning with the objective to ensure each listener is added only once. This is a correct use ofConcurrentMap's capabilities.- 43-43: Calling
addListenerswithother.getListeners()in themergemethod to combine listeners from anotherDataProviderHolderis a straightforward and effective way to merge listener collections without introducing duplicates.
krmahadevan
force-pushed
the
bugfix/3045
branch
from
e5c2b4e
to
4a811ab
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- CHANGES.txt (1 hunks)
- testng-core/src/main/java/org/testng/DataProviderHolder.java (4 hunks)
- testng-core/src/test/java/test/dataprovider/DataProviderTest.java (3 hunks)
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderListener.java (1 hunks)
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderTestClassSample.java (1 hunks)
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderWithoutListenerTestClassSample.java (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- CHANGES.txt
- testng-core/src/main/java/org/testng/DataProviderHolder.java
- testng-core/src/test/java/test/dataprovider/DataProviderTest.java
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderListener.java
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderTestClassSample.java
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderWithoutListenerTestClassSample.java
| runTest(DataProviderWithoutListenerTestClassSample.class, DataProviderListener.class.getName()); | ||
| } | ||
|
|
||
| private static void runTest(Class<?> clazz, String listener) { |
There was a problem hiding this comment.
Class<? extends TestngListener> could be expressive.
There was a problem hiding this comment.
No. The first parameter is actually the test class and not the listener. The listener is being passed in as a String
There was a problem hiding this comment.
Yes. That's my point. Why do you use a string?
There was a problem hiding this comment.
TestNG is either ways going to accept a string. So why not ?
There was a problem hiding this comment.
It is a question of style and api design. You should not exposed untyped object when it is possible to avoid it.
String is not a strong type because you can put everything into it.
Here Class is a better choice and you will not have to check inputs.
Just do the string conversion the later you can.
There was a problem hiding this comment.
@juherr - Couple of things
- This is an existing API in the
XmlSuiteclass and I am not adding a new one.XmlSuitewill only accept strings. - The method in question is being used by 2 test methods with the sole intention of avoiding code duplication. So the rationale of exposing any untyped object or api design does not hold valid here.
- I will still have to check for inputs, because the reusable method
runTestis going to set a fully qualified class name toXmlSuiteONLY if a listener was passed. So if aClass<?>was passed, then I would be sending in null and thus doing a null check and if a string is being sent, i do a empty check.
So I am not convinced that the change you are suggesting is going to add any value here because this is just within the scope of a reusable method that is being shared by 2 test methods with the sole intention of avoiding code duplication.
There was a problem hiding this comment.
The point is not modifing current api
And I agree the value add here, in this specific context, is very low.
It is a general advice which I think should be an habbit.
I don't understand the argument to not doing it.
There was a problem hiding this comment.
@juherr - It wasn't an argument. I was genuinely trying to understand the rationale. I agree with you on the fact that it would need to be a general good habit. My contention was that in this case it wasn't needed.
Either ways, it turns out that we both missed one part in that method :) I was using a string to pass the listener name, but was actually resorting to using a hard coded listener name always. I have now fixed it by replacing it with a boolean which indicates if a listener should or should not be wired in or not.
There was a problem hiding this comment.
My apologies if it sounded as if I was on an argumentative mode. That was not my intention.
testng-core/src/test/java/test/dataprovider/issue3045/DataProviderListener.java
Outdated
Show resolved
Hide resolved
krmahadevan
force-pushed
the
bugfix/3045
branch
from
4a811ab
to
94c4d2b
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- CHANGES.txt (1 hunks)
- testng-core/src/main/java/org/testng/DataProviderHolder.java (4 hunks)
- testng-core/src/test/java/test/dataprovider/DataProviderTest.java (3 hunks)
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderListener.java (1 hunks)
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderTestClassSample.java (1 hunks)
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderWithoutListenerTestClassSample.java (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- CHANGES.txt
- testng-core/src/main/java/org/testng/DataProviderHolder.java
- testng-core/src/test/java/test/dataprovider/DataProviderTest.java
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderListener.java
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderTestClassSample.java
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderWithoutListenerTestClassSample.java
krmahadevan
force-pushed
the
bugfix/3045
branch
from
94c4d2b
to
c47c9bd
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- CHANGES.txt (1 hunks)
- testng-core/src/main/java/org/testng/DataProviderHolder.java (4 hunks)
- testng-core/src/test/java/test/dataprovider/DataProviderTest.java (3 hunks)
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderListener.java (1 hunks)
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderTestClassSample.java (1 hunks)
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderWithoutListenerTestClassSample.java (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- CHANGES.txt
- testng-core/src/main/java/org/testng/DataProviderHolder.java
- testng-core/src/test/java/test/dataprovider/DataProviderTest.java
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderListener.java
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderTestClassSample.java
- testng-core/src/test/java/test/dataprovider/issue3045/DataProviderWithoutListenerTestClassSample.java
Closes #3045
Fixes #3045 .
Did you remember to?
CHANGES.txt./gradlew autostyleApplyWe encourage pull requests that:
If your pull request involves fixing SonarQube issues then we would suggest that you please discuss this with the
TestNG-dev before you spend time working on it.
Note: For more information on contribution guidelines please make sure you refer our Contributing section for detailed set of steps.
Summary by CodeRabbit
java.lang.IllegalStateException) occurring when results per method were empty.