Code clean-up #3027
Code clean-up #3027
Conversation
krmahadevan
commented
Dec 28, 2023
krmahadevan
commented
- Use finals as and when applicable
- delete commented out code
- convert inner classes into static
testng-core/src/main/java/org/testng/internal/ClonedMethod.java
Outdated
Show resolved
Hide resolved
testng-core/src/main/java/org/testng/internal/TestNGClassFinder.java
Outdated
Show resolved
Hide resolved
| @@ -54,7 +54,7 @@ public void testCloneIfContainsTestsWithNamesMatchingAnyChildSuites( | |||
| dataProvider = "getData") | |||
| public void testCloneIfContainsTestsWithNamesMatchingAnyNegativeCondition( | |||
| XmlSuite xmlSuite, List<String> names) { | |||
| TestNamesMatcher testNamesHelper = new TestNamesMatcher(xmlSuite, names); | |||
| new TestNamesMatcher(xmlSuite, names); | |||
There was a problem hiding this comment.
No it's not. The constructor of TestNamesMatcher internally does some edit checks and throws an exception and this test is vetting out that the exception is actually getting thrown or not.
There was a problem hiding this comment.
But that is a side effect and breaks oop.
Could you considere to move the logic into a method used here and into the constructor.
There was a problem hiding this comment.
I didnt understand you here @juherr. The current implementation invokes a method named cloneIfContainsTestsWithNamesMatchingAny
This call is being done via constructor invocation. Are you suggesting that we move this call to some sort of a lazy invocation and leave the constructor to just member initialisation ? I believe that this rule is currently violated in multiple places within TestNG.
There was a problem hiding this comment.
The object construction is useless here because we don't use any object.
The only reason to use it is because we know there are some side effects.
It will be more logical to call a real function instead.
It could be fixed every where possible
There was a problem hiding this comment.
It will be more logical to call a real function instead.
This wont be possible, because then it would change the semantics of what this class represents. We will have to alter all calls to explicitly call this new validation method. I believe that the validation is part of this class internals and the outside caller doesn't need to know anything about it.
Validating input parameters as part of Object instantiation, AFAIK is NOT a huge problem. It's a general practical to add semantic validations for input parameters passed into the constructor. This method cloneIfContainsTestsWithNamesMatchingAny is also doing such a semantic validation.
So can you please help me understand what is the real concern here so it can be addressed?
There was a problem hiding this comment.
@juherr - Can you please let me know if this concern is addressed and if this PR can be proceeded further with approval and merge?
There was a problem hiding this comment.
Calling a method sounds better but keep as it is for the moment
testng-core/src/test/java/org/testng/xml/internal/TestNamesMatcherTest.java
Show resolved
Hide resolved
krmahadevan
force-pushed
the
additional_code_refactorings
branch
from
00f4051
to
256573e
Compare
krmahadevan
force-pushed
the
additional_code_refactorings
branch
from
256573e
to
3a58d94
Compare
| @@ -152,7 +153,8 @@ private void processMethod( | |||
|
|
|||
| private static boolean excludeFactory(FactoryMethod fm, ITestContext ctx) { | |||
| return fm.getGroups().length != 0 | |||
| && ctx.getCurrentXmlTest().getExcludedGroups().containsAll(Arrays.asList(fm.getGroups())); | |||
| && new HashSet<>(ctx.getCurrentXmlTest().getExcludedGroups()) | |||
There was a problem hiding this comment.
Please add a comment that explain why hashset is used here.
| @@ -54,7 +54,7 @@ public void testCloneIfContainsTestsWithNamesMatchingAnyChildSuites( | |||
| dataProvider = "getData") | |||
| public void testCloneIfContainsTestsWithNamesMatchingAnyNegativeCondition( | |||
| XmlSuite xmlSuite, List<String> names) { | |||
| TestNamesMatcher testNamesHelper = new TestNamesMatcher(xmlSuite, names); | |||
| new TestNamesMatcher(xmlSuite, names); | |||
There was a problem hiding this comment.
Calling a method sounds better but keep as it is for the moment
* Use finals as and when applicable * delete commented out code * convert inner classes into static
krmahadevan
force-pushed
the
additional_code_refactorings
branch
from
3a58d94
to
e59b6cc
Compare
