Skip to content
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

By EasonYi: #2248

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

EasonYi
Copy link

@EasonYi EasonYi commented Feb 18, 2020

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.

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
@krmahadevan
Copy link
Member

@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 krmahadevan requested a review from juherr February 18, 2020 09:08
@juherr juherr requested a review from krmahadevan February 18, 2020 09:17
Copy link
Member

@juherr juherr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the change but I agree with @krmahadevan : could you propose one pull request by subject?

(edited after @krmahadevan comment)

@krmahadevan
Copy link
Member

@juherr -

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

TestNG 7.1.0 needs JDK 8 as the minimum JDK

@juherr
Copy link
Member

juherr commented Feb 18, 2020

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

@krmahadevan
Copy link
Member

@juherr

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

Requirements TestNG requires JDK 7 or higher.

@juherr
Copy link
Member

juherr commented Feb 18, 2020

@krmahadevan yes

@EasonYi
Copy link
Author

EasonYi commented Feb 18, 2020

@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

@krmahadevan
Copy link
Member

@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.

Copy link
Member

@krmahadevan krmahadevan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a nutshell, request you to please help do the following:

  1. Rebase off of master to fix the merge conflicts.
  2. Break this PR into 3 PRs (since we are addressing 3 different issues in this one PR)

@EasonYi
Copy link
Author

EasonYi commented Feb 20, 2020

@krmahadevan

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants