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

feat: reworks testng7 support #261

Merged
merged 1 commit into from Aug 12, 2020
Merged

Conversation

Zlika
Copy link
Contributor

@Zlika Zlika commented Jul 15, 2020

Short description of what this resolves:

This PR is a rework of the testng7 support added in #232.
The problems with the original testng7 support are:

  • It's not working (cf. arquillian-testng.jar does not include jcommander when using TestNG 7 #258).
  • It creates unnecessary code duplication (3 modules are copied for a single line of code changed).
  • It breaks compatibility with existing code when migrating from testng <7 to testng7 (package name changed).
  • It makes people believe that the new modules can only be run using testng7 (whereas it is compatible starting from testng 6.0.1 released 9 years ago!).

Changes proposed in this pull request:

  • The "new" testng7 modules are removed.
  • The code now tests the existence of the ITestNGMethod.getMethod() method (testng <7) and if it does not exist call ITestNGMethod.getConstructorOrMethod() (testng>=6.0.1) instead.

This way: no more code/module duplication, and we are compatible with all versions of testng (if compatibility with testng versions older than 6.0.1 is not required, no need for reflection, just replace ITestNGMethod.getMethod() with ITestNGMethod.getConstructorOrMethod()).
I tested this PR on a project using testng7 and it works (no need for manually adding testng/jcommander as a compile dependency like the workaround proposed in #258).

Fixes #258

@Zlika
Copy link
Contributor Author

Zlika commented Jul 22, 2020

Hello! Anyone to review this PR?

@Zlika
Copy link
Contributor Author

Zlika commented Jul 29, 2020

Hi @bartoszmajsak ! Could you please review this PR? I really need Arquillian to be compatible with TestNG7.

@bartoszmajsak
Copy link
Member

Hey, sure. I'll take care of it today.

Copy link
Member

@bartoszmajsak bartoszmajsak left a comment

Choose a reason for hiding this comment

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

Many thanks for this clean-up. Much appreciated. I will cut the release today with few other PRs.

@bartoszmajsak bartoszmajsak changed the title Rework testng7 support feat: reworks testng7 support Aug 12, 2020
@bartoszmajsak bartoszmajsak merged commit 415d0bb into arquillian:master Aug 12, 2020
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.

arquillian-testng.jar does not include jcommander when using TestNG 7
2 participants