Allow ITestObjectFactory injection via listeners #2677
Conversation
|
@juherr i also had the same feel. I dont have an answer on how to get it streamlined. Maybe we can take it as a separate pull request ? On a bare essential i think we should only query the suite object for a factory because it is getting injected with a factory object from TestNG. But the only challenge i saw is when a user sets a test object factory via their test class. Right now its a bit convoluted. I would prefer to deal with it as a separate PR and not mix it with this feature PR. |
|
@krmahadevan Agree to track the architecture point into another issue. |
krmahadevan
force-pushed
the
fix_2676
branch
from
2c4e019 to
5740349
Compare
|
@juherr - I have updated the PR based on your review comments. Please help take a look. |
I have now created a new issue for tracking the architectural discrepancies as part of #2679 |
| result = new Object[] {Dispenser.newInstance(m_objectFactory).dispense(attributes)}; | ||
| result = | ||
| new Object[] { | ||
| Dispenser.newInstance(m_testContext.getSuite().getObjectFactory()) |
There was a problem hiding this comment.
Why not use the same logic here? https://github.com/cbeust/testng/pull/2677/files#diff-73ef03ca958970a8d4a3f3c0764700a861f31f92a10d0b46930671bee51dfba8R98-R102
Maybe a private method could help.
There was a problem hiding this comment.
@juherr - I didnt understand this comment and the link you shared is taking me to the same class. Can you please help me understand ?
There was a problem hiding this comment.
Sorry, I was not explicit enough.
You have 2 different ways to create a dispenser in the same class. Is it expected? If it is expected, could you add a comment that explains why? If it is not expected, you can share a same builder method.
There was a problem hiding this comment.
@juherr - Yes you are right. One place is for the JUnit flow but I dont even see that codepath being executed and so I dont know why it exists even. I was first thinking that, this particular code path is going to get executed when we have junit attribute in suite set to true and I am running a bunch of JUnit tests using TestNG. But I noticed that the tests don't even take that path. It may be dead code but I cannot remove it until I know for sure. I will dig into it deeper separately and not in this PR.
There was a problem hiding this comment.
From looking at the code, it looks like we perhaps never supported the notion of a test object factory for JUnit tests, because the codepath (when I execute a JUnit annotated test using TestNG) is going such that, JUnit is internally invoking the constructor of the test class and our objectfactory is not even coming into picture. I think if there are no other comments on this PR (which is to fix test objectfactory to be set for TestNG tests via listeners) we should merge this PR and deal with cleaning up the test object factory impact for JUnit tests separately (and maybe even remove the irrelevant code sections. )
Maybe I dont know enough but this is my first impression from what I see
There was a problem hiding this comment.
If you ask me, I will propose to drop the junit support and advice to use https://github.com/junit-team/testng-engine when users ask for running both junit and testng.
There was a problem hiding this comment.
@juherr sure. So can we go ahead with merging this PR ?
There was a problem hiding this comment.
just add a comment that link to the current discussion ;)
There was a problem hiding this comment.
I went ahead and captured this as a defect and included all the details #2683
Closes #2676
Fixes #2676 .
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.