-
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
Upgrade dependencies, Upgrade to JDK17 #2747
Upgrade dependencies, Upgrade to JDK17 #2747
Conversation
@hduerkop - the minimum JDK version of TestNG cannot yet be more than 11. We havent yet released one version that uses jdk11 as default minimum version. So please exclude the JDK bump from your changeset and update this PR. |
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.
LGTM except the jdk target (we want to support the previous and the current LTS : 11 + 17)
testng-core/src/test/java/org/testng/internal/invokers/ParameterHandlerTest.java
Outdated
Show resolved
Hide resolved
import org.testng.IReporter; | ||
import org.testng.ITestNGListener; | ||
import org.testng.TestNG; | ||
import org.testng.annotations.AfterClass; | ||
import org.testng.annotations.BeforeClass; | ||
import org.testng.annotations.DataProvider; | ||
import org.testng.annotations.Test; | ||
import org.testng.reporters.EmailableReporter; | ||
import org.testng.reporters.EmailableReporter2; | ||
import test.SimpleBaseTest; | ||
|
||
public class EmailableReporterTest extends SimpleBaseTest { |
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.
Same goes for this as well. Any specific reason behind us removing off this test that works with a custom security manager?
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.
I removed it because of https://openjdk.java.net/jeps/411 .
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.
@hduerkop - Please revert it back since we are still saying that we are going to be JDK11 as the minimum JDK but the security manager has been deprecated in JDK17. So we should perhaps leave it there as is until TestNG moves into JDK17 or above as its minimum JDK.
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.
I have to disagree here since JDK17 is the current LTS and is or soon will be dominant on build platforms
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.
@hduerkop - if testng has jdk11 as its minimum jdk then we will need to have everything that exists in jdk11 including tests that test features which are relevant for jdk11 as well.
I agree that build systems atleast on the open source world may move forward but we need to be cognizant of user needs and cannot enforce this upon our users.
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.
It is not an enforcement, but an option to use JDK17 this way - 11 and 17 work this way.
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.
What was the initial objective of the test? Is it another way to test 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.
LGTM but need a major release. Maybe we need to make a new release before merging.
|
||
tasks.withType<KotlinCompile>().configureEach { | ||
kotlinOptions { | ||
jvmTarget = "11" |
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.
Is it possible to manage this version in the gradle properties file?
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.
I don't think so, but feel free to improve it
@krmahadevan do you plan a release soon ? |
@hduerkop - Can you please help fix the merge conflicts so that this PR can be merged ? |
@juherr - We can do that. 7.5 was released in Jan and we do have some bug fixes that can be released along with the JDK11 requirements as well. I will wait for this PR to be mergeable (after conflicts have been resolved) and then maybe we can go through with a release. |
@krmahadevan it looks mergeable to me. |
Fixes # .
Did you remember to?
CHANGES.txt
./gradlew autostyleApply
We 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.