testng icon indicating copy to clipboard operation
testng copied to clipboard

By EasonYi:

Open EasonYi opened this issue 6 years ago • 8 comments

New: Allow the same method in one class for more than one time New: Allow the same class in one test for more than one time New: Upgrade to Java 8 Fixed: Adjust the default parallel mode of SuiteRunner to TESTS Fixed: Wrong implementation about parameters overriding Notice: Since a lot of things have been changed, but only XML style was tested, that is JUnit and others are NOT tested by now, and you can find the tests in src/test/java/test/duplicate directory

Fixes # .

Did you remember to?

  • [ Yes] Add test case(s)
  • [Yes ] Update CHANGES.txt

We encourage pull requests that:

  • Add new features to TestNG (or)
  • Fix bugs in TestNG

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.

EasonYi avatar Feb 18 '20 07:02 EasonYi

@EasonYi - Couple of things.

  1. There are merge conflicts with this PR. You would need to help fix that.
  2. The changes seems to be huge and contains multiple changes related to multiple things. It would be great if you could please help break this down into multiple PRs each of which fixes one specific thing.

krmahadevan avatar Feb 18 '20 09:02 krmahadevan

@juherr -

BTW, we don't plan to switch to java8 for the moment.

TestNG 7.1.0 needs JDK 8 as the minimum JDK

krmahadevan avatar Feb 18 '20 09:02 krmahadevan

@krmahadevan oops :D I didn't remember and the site (and gradle) is not up-to-date :p

juherr avatar Feb 18 '20 09:02 juherr

@juherr

You mean in this page : https://testng.org/doc/

Requirements TestNG requires JDK 7 or higher.

krmahadevan avatar Feb 18 '20 11:02 krmahadevan

@krmahadevan yes

juherr avatar Feb 18 '20 13:02 juherr

@EasonYi - Couple of things.

  1. There are merge conflicts with this PR. You would need to help fix that.
  2. The changes seems to be huge and contains multiple changes related to multiple things. It would be great if you could please help break this down into multiple PRs each of which fixes one specific thing.

@krmahadevan - Thank you for your comments!

I've made this PR based on 6.14.3 version, and the most changes are about the feature which allowing the same method in one class for more than one time and the same class in one test for more than one time, which is helpful when we want to make a complex test with the TestNG XML file.

Since the change need the "default implementation of interface" which is available on Java 8, so I changed the source level to it, maybe the target level can still remain Java 7.

And the following changes are very small.

  • Fixed: Adjust the default parallel mode of SuiteRunner to TESTS
  • Fixed: Wrong implementation about parameters overriding

EasonYi avatar Feb 18 '20 14:02 EasonYi

@EasonYi

I've made this PR based on 6.14.3 version,

After that we have gone through 2 more releases. So you would need to help rebase your changes off of master, fix the merge conflicts, before we can take a look at this PR.

and the most changes are about the feature which allowing the same method in one class for more than one time and the same class in one test for more than one time, which is helpful when we want to make a complex test with the TestNG XML file.

Can you please create an issue and include details in terms of what the problem is, along with a sample. That way when we send out release notes, we can give our users some context into the sort of changes that are going in.

Since the change need the "default implementation of interface" which is available on Java 8, so I changed the source level to it, maybe the target level can still remain Java 7.

TestNG moved to using JDK8 as the minimum JDK as of TestNG 7.0.0. So we are good there and you don't need to flip back to Java7.

And the following changes are very small.

Fixed: Adjust the default parallel mode of SuiteRunner to TESTS Fixed: Wrong implementation about parameters overriding

Sure. But from a review point, it would be really helpful to us, if you can restrict a PR to just one issue. So for the above two issues, you could raise a separate PR and we can have it reviewed and merged.

The more small and focussed a changeset is, the more easier it is to reason with it and get it merged.

Hope that adds some clarity to my ask.

krmahadevan avatar Feb 19 '20 04:02 krmahadevan

@krmahadevan

Thank you! I understand your suggestions and I'll do it when I'm free in the future.

EasonYi avatar Feb 20 '20 01:02 EasonYi