-
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
Move SimpleBaseTest to be Kotlin based #2703
Conversation
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.
With Kotlin, you'll have to think outside the box ;)
- no more function overriding
- no need for static
val TEST_RESOURCES_DIR = "test.resources.dir" | ||
|
||
@JvmStatic | ||
fun run(vararg testClasses: Class<*>) : InvokedMethodNameListener { |
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.
@JvmStatic
fun run(vararg testClasses: Class<*>) = run(false, *testClasses)
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.
the function can be removed because you can set a default value on skipConfiguration
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.
This function is being used by child classes and I am trying to keep the changeset to as minimal as possible.
This will need to exist as long as we mix Java with Kotlin. All the tests are java based and they expect to use the static methods from the base class and this is the only way in which I could get this done. |
Why not having Kotlin base for Kotlin tests and keep Java base for Java tests? |
There are no kotlin tests at all in the codebase. So not sure why we need to have two base classes. If the intent is to eventually move TestNG into a complete Kotlin project then we should have just 1 base class and so I chose to move the base class to Kotlin based. I will be raising more PRs to migrate the java based tests to kotlin and then we can start moving one module at a time into kotlin. |
@juherr - Please let me know if this is not the intended direction and I will abort. I was under the impression that we are looking at eventually moving entire TestNG into a kotlin based project and that is why i started the migration with the tests because they are a lot more simpler and I am still learning Kotlin (this is my 3rd day of learning it). |
In fact, I'm not sure about your strategy. |
I mean using java from kotlin is easy but I will avoid using kotlin from java if I can. |
So does it mean that TestNG is NOT going to be migrating over to be a complete kotlin project ? Is that what you are hinting at ? @cbeust - Wanted to hear your views as well.
My PR is also doing the same thing. It migrated the SimpleBaseTest.java into a kotlin variant.
My PR is exactly facilitating this by having java based child classes which represent our unit tests, to extend from a Kotlin based base class so that we will have Java extends Kotlin. If we go with migrating child test classes into Kotlin, it would end up with Kotlin class extends Java base class. |
I understand the goal but I think the strategy is not appropriate:
I mean there is no problem if you move some test classes from Java to Kotlin and keep the parent class in Java.
Will be or not. The goal is not removing Java but adding Kotlin. I feel the api/spi will stay in Java for a long time. |
So then this PR is aligned with the goal because the existing Java based test classes will extend the new Kotlin based base class.
This is what will happen if I migrate the test classes first and then the base class at the end. So I think from the approach perspective I am aligned to what you are suggesting as well which is Migrate such that we always end up with Java consumes Kotlin, but never such that Kotlin consumes Java
This sounds contradictory to the statement that using Kotlin from Java will be hackish because migrating all the tests into Kotlin and finally migrating the base class is exactly going to lead us into this situation. All said and done, my larger question is, given the fact that the base class is housing only bare essential functionality, can you please help me understand what is the challenge you are seeing that is stopping us from merging this PR, because I am not clear.
Not necessarily true. If TestNG works on porting all the Java code into Kotlin, then atleast from the codebase perspective there wont be any Java. True an end-user will still see Java when they consume TestNG as a dependency, but that's the same case for all other JVM based projects (for e.g., RestAssured )
I feel that we are going to burden our users with a Kotlin dependency which is NOT being used anywhere in the codebase. But if we first work on moving all the tests into Kotlin and then introduce Kotlin as a direct dependency as a result of TestNG migrating one of its modules into Kotlin would make sense. Just for my understanding this question.. A Java user will be able to consume TestNG without any glitches even after TestNG moves into a Kotlin based project right? If this statement is true, then I dont think our users need to be bothered about what dependencies that TestNG brings in, because their botheration should be the functionality. TestNG has never given any assurance anywhere that it would refrain from bringing in any dependencies (or) will ensure it does not add any dependencies. It's a library like any other library. |
e195b0d
to
334d06b
Compare
@juherr - Can you please help take another look at this PR and let me know if there is anything else that needs to be fixed before it can be merged ? |
Please, don't be blocked by this PR. |
@juherr - Can you please tell me what is the real concern? Because the steps dont matter since eventually its going to land us in the same end state ( all unit tests in kotlin ) and this is actually not a big change. It hasnt added any code change in src/main/java. So am really curious to know the real concern. |
I don't think you need to set as a goal "Move TestNG 100% to a Kotlin project". Make it happen organically, start writing new classes in Kotlin. If some existing classes/algorithms look like they might be more elegant in Kotlin and they are appropriately tested, go ahead and rewrite them in Kotlin. |
@cbeust - thanks for chiming in. I am right now more comfortable on porting tests because it has a triple advantage.
My intent is also NOT to do 100% porting ( but if time permits, then why not ) I need to first get familiar with Kotlin enough to be able to decide what to touch/migrate and what not to. I am still trying to understand what is wrong with this PR because am not able to understand that part. So am waiting to hear from @juherr on that. In the meantime @cbeust please let me know your thoughts on this PR as well. |
334d06b
to
2f1c1e3
Compare
Refactored one of the base classes for all tests To be Kotlin based. Altered the build file to define test compilation Order such that: * Kotlin code gets built first before Groovy test code * Kotlin build output directory gets added to the Classpath of groovy test compilation.
2f1c1e3
to
2cb6b7f
Compare
Looking good, @krmahadevan, you're getting the hang of this! :-) |
The PR is clearly better now, good job! 👍 I have/had some doubts because I think "good" Kotlin doesn't override methods when possible but use default value instead. BTW, LGTM now. |
fun run(vararg testClasses: Class<*>) = run(false, *testClasses) | ||
|
||
@JvmStatic | ||
private fun run(skipConfiguration: Boolean, tng: TestNG) = |
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.
Ok to keep the function for compatibility purpose but I think private fun TestNG.run(skipConfiguration: Boolean)
is "better" Kotlin.
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. Agreed. Will get to it when the Kotlin migration is complete because either ways a round of refactoring of the code will be required to leverage more of the Kotlin syntax. But I think for now its going to be as good as premature optimisation because we still have Java tests that need to be moved over. It wouldnt matter even if the tests are migrated first because they will still use the same syntax of the Java base class which when refactored will start affecting all the tests.
To me its the same effort irrespective of which way we migrate things from. Since I started with the base class, I would request you to please help me proceed with the approach (given the fact that nothing is breaking and this is only incremental refactoring)
|
||
@JvmStatic | ||
fun run(skipConfiguration: Boolean, vararg testClasses: Class<*>) = | ||
run(skipConfiguration, create(*testClasses)) |
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.
From a Kotlin POV, I will be more confortable with: TestNG().run(skipConfiguration, *testClasses)
or create(*testClasses).run(skipConfiguration)
because we are supposed to run test against the testng instance.
The Java parent classe doesn't do that because extensions are not possible in Java.
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.
@juherr - From what I have read about extension functions in Kotlin, they get translated into static
funtions in the Java world, with the type (to which they are added as extension funtions) getting passed implicitly as the first parameter to the static method.
The problem with this is that now all the Java sub classes will be required to start adding the extra static parameter which is going to add a whole lot of changes.
So can we for now just stick to the @JvmStatic
way of exposing the companion methods and then move forward with migrating the tests into Kotlin. I will take care of incremental refactoring of the tests to be more Kotlinish, once the migration is complete, because that is going to be a lot more easier.
} | ||
|
||
@JvmStatic | ||
protected fun createXmlTestWithPackages( |
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.
From a Kotlin test, I think xmlSuite.createXmlTestWithPackages(...)
is more logical than createXmlTestWithPackages(xmlSuite, ...
.
You can fix it by adding an extension.
Same logic for the other facture functions.
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.
@juherr - From what I have read about extension functions in Kotlin, they get translated into static
funtions in the Java world, with the type (to which they are added as extension funtions) getting passed implicitly as the first parameter to the static method.
The problem with this is that now all the Java sub classes will be required to start adding the extra static parameter which is going to add a whole lot of changes.
So can we for now just stick to the @JvmStatic
way of exposing the companion methods and then move forward with migrating the tests into Kotlin. I will take care of incremental refactoring of the tests to be more Kotlinish, once the migration is complete, because that is going to be a lot more easier.
Its going to be the same effort @juherr and IMO we are not going to be gaining anything with doing the migration from bottom up (versus the top down approach). We will need to multiple iterations of refactoring before we can bring the code to be proper Kotlinish and I am ready for doing that. Migrating the tests will still end up causing a last mile change at the base class when we migrate it to Kotlin. In my case, we are kind of doing the same thing but just getting everything into Kotlin, before we refactor the base class once again. I am ready to take on that effort of doing it. |
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.
Ok for the migration for the base class even if it won't be used as parent class of Kotlin tests.
The main reason is Kotlin is an opportunity to have a new fresh eye and it will create new needs/usages which should not be based on current Java habits.
@juherr - Thank you. My intent for the tests is 3 folded:
Also create some basic documentation in terms of adding unit tests that can be followed by contributors as well. I will work on getting all of this done, as and when I get time. My personal goal is to be able to get most of this done by the next release (and also ensure that we dont drag a release because of this exercise 😁 ) |
Refactored one of the base classes for all tests to be Kotlin based.
Altered the build file to define test compilation order such that: