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

[SUREFIRE-2064] Allow all supported values of [parallel] option #517

Merged
merged 1 commit into from Apr 23, 2022

Conversation

sbabcoc
Copy link
Contributor

@sbabcoc sbabcoc commented Apr 8, 2022

This pull request revises the handling of the TestNG 7.4+ [parallel] option so that it accepts all valid values.

Following this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a JIRA issue filed
    for the change (usually before you start working on it). Trivial changes like typos do not
    require a JIRA issue. Your pull request should address just this issue, without
    pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [SUREFIRE-XXX] - Fixes bug in ApproximateQuantiles,
    where you replace SUREFIRE-XXX with the appropriate JIRA issue. Best practice
    is to use the JIRA issue title in the pull request title and in the first line of the
    commit message.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean install to make sure basic checks pass. A more thorough check will
    be performed on your pull request automatically.
  • You have run the integration tests successfully (mvn -Prun-its clean install).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

@sbabcoc
Copy link
Contributor Author

sbabcoc commented Apr 8, 2022

In this PR, I fixed issues with handling of the [parallel] option in the existing implementation. However, I've since noticed a few other issues with the existing implementation:

  • The superclass configuration handler is completely overridden, causing almost every option to be ignored.
  • If no [threadcount] option is specified, the existing implementation is imposing a default value of 1. This is incorrect and unnecessary, as TestNG supplies a default value of 5 for this parameter.
  • No unit tests were implemented for this class. These were potentially skipped because the version of TestNG specified by this project's dependencies (v5.10) pre-dates the introduction of the supported features.

These deficiencies should be addressed to maintain the quality of the Surefire project. I'll switch this PR to "draft" mode and see what I can do about these issues.

@sbabcoc sbabcoc marked this pull request as draft April 8, 2022 16:18
@sbabcoc sbabcoc marked this pull request as ready for review April 8, 2022 19:59
@sbabcoc
Copy link
Contributor Author

sbabcoc commented Apr 8, 2022

I've completely re-implemented the TestNG740Configurator class. The new implementation...

  • ... overrides the handling of the [parallel] option to resolve the incompatibility with TestNG 7.4+.
  • ... acquires the default value of the [threadcount] option to avoid superclass hardcoding to 1.
  • ... invokes the superclass handler to allow processing of other supported options.

I retained code from the existing implementation to load the ParallelMode enumeration, convert the specified option to its corresponding constant, and set the XmlSuite parallel mode setting. I added exception handling and conditional blocks to toughen it up.

@sbabcoc
Copy link
Contributor Author

sbabcoc commented Apr 9, 2022

Fixes implementation of #339

@sbabcoc
Copy link
Contributor Author

sbabcoc commented Apr 9, 2022

@Tibor17 Here are my revisions to enable support for TestNG 7.4+

@Tibor17 Tibor17 self-requested a review April 9, 2022 14:54
@Tibor17
Copy link
Contributor

Tibor17 commented Apr 10, 2022

@sbabcoc
@SykApps
@slachiewicz
Now the OOP looks better in the latest commit. WDYT?
We can better concentrate on the details implemented in both methods.
@slachiewicz This would help us when we would perform the removal of TestNG 5.x and we can much better merge the logic in a new objective design across the version 6.x and 7.x. Currently, some options, like setJunit, are lost and that's we did not controll Configuration in pull requests, we only accepted and that's the penalty.

@slachiewicz
Copy link
Member

@slawekjaranowski

Copy link
Member

@slawekjaranowski slawekjaranowski left a comment

Choose a reason for hiding this comment

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

lgtm.

@sbabcoc
Copy link
Contributor Author

sbabcoc commented Apr 12, 2022

FUTURE: As an alternative to the version-specific custom parsing currently employed by the Surefire plugin, it might be worth exploring the possibility of using TestNG's Parser class instead. This can be instantiated with any input stream, including a ByteArrayInputStream. This would transform all option processing to a version-agnostic conversion of Surefire configuration settings to corresponding TestNG XML format, instantiating a Parser, and calling the parse() method.

@Tibor17
Copy link
Contributor

Tibor17 commented Apr 12, 2022

@sbabcoc
We are not doing the refactoring due to reflection. Reflection is a minimum problem for the developer. The main problem in this Configuration is the fact that we relied on the quality of external contributors that this part of code was out of our control and the quality was really low, even buggy. This PR is non technical fix in real. We will continue with @slawekjaranowski after this PR and we will serve to our contributors and users with higher quality. The next is the removal of TestNG 5.x and the reason behind it is the fact that 5.x does not work on JDK 15+ and the version is over 10 years old. We will remove Surefire provider surefire-junit4 because it has the same features as surefire-junit47, we save 25% of our cost, we speed up the CI, and we guess that the users won't use obsolete and narrow range of JUnit [4.0, 4.7) and mostly they use 3.8.2 (surefire-junit3), 4.8.2 (surefire-junit47), 4.12 and 4.13.x, we wil have an unsupported gap 4.0 - 4.7.
Altogether, this this activity is much more than a little tiny refactoring.

@sbabcoc
Copy link
Contributor Author

sbabcoc commented Apr 12, 2022

@Tibor17 I find it astonishing that anyone still uses JUnit 3.8.2. I continue to support JUnit 4.13.2 because I still want my libraries to run on Java 1.7. (I actually use byte code generation via Byte Buddy to add test lifecycle hooks at runtime.)
Would it be worth the effort to assemble a POC for how the TestNG configuration could be performed via the Parser class?
Even in TestNG 5.1, there was a publicly-accessibly command line parser. The class name and interface changed, but this could still be leveraged to eliminate version-specific processing from Surefire.

@Tibor17
Copy link
Contributor

Tibor17 commented Apr 14, 2022

@sbabcoc
If I remember correctly, junit 3 is or was used by android. The same provider supports POJO tests without any framework.
Feel free to make the PoC. If it is small enough, you can do it in the comment or use Gist.
We transfer the config parameter via properties file, and this is the same for all providers. The XML marshaller/unmarshaller would not fit to this approach because one provider would be different and handled differently.
If the Parser or xml was embedded within the module surefire-testng only then the surrounding modules would not be touched which would be interesting for us.

@sbabcoc
Copy link
Contributor Author

sbabcoc commented Apr 15, 2022

I'll see what I can come up with.

@Tibor17
Copy link
Contributor

Tibor17 commented Apr 16, 2022

@sbabcoc
Pls squash these commits to one and we wil make a final review.

The situation is as follows.
We are in a little progress with our internal wishes and we have to fix broken M6.
Additionally, I have backported cca 20 bug fixes from M6 to 2.22.3 but also the TestNG 7.4 bug fix and this one is missing.
I guess @slawekjaranowski would continue on fixing Configuration implementation and removal of 5.x. The fix of impl of Configuration interface is important for M7 and it would be nice in 2.22.3.

@sbabcoc
Copy link
Contributor Author

sbabcoc commented Apr 16, 2022

@Tibor17: The revisions in this PR have been squashed to a single commit. Thanks for all your efforts on this project!

@Tibor17
Copy link
Contributor

Tibor17 commented Apr 16, 2022

@slawekjaranowski Can you pls help me making a review as well? Pls participate.
@sbabcoc Feel free to work on the Parser, pls don't stop. We can introduce it in any PR or here if we are fast.

throw new TestSetFailedException( "Unsupported TestNG parallel setting: "
+ parallel + " ( only METHODS or CLASSES supported )" );
// convert and apply [threadcount] setting
suite.setThreadCount( Integer.parseInt( threadCount ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

Previously it was threadCountAsString == null ? 1 : .... What would happen if parallel=classes but the ThreadCount is not set? How is TestNG implemented? It has default value 1?

Copy link
Contributor

@Tibor17 Tibor17 Apr 16, 2022

Choose a reason for hiding this comment

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

I found the default value in TestNG:
private int m_threadCount = 5;
We should use the same line as before, means suite.setThreadCount( threadCount == null ? 1 : parseInt( threadCount ) )

Copy link
Contributor Author

@sbabcoc sbabcoc Apr 17, 2022

Choose a reason for hiding this comment

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

If all supported versions of TestNG provide a default for this setting, it may be best to not inject a default here. Just let TestNG provide its own default value. That's how the new code for TestNG 7.4+ behaves. I don't know the history of the original code to understand why this was coded to inject a default value of 1. (What's the benefit of specifying parallel execution if the thread count is 1?)
If the default implementation is changed so that it behaves like the new override, this override can be removed.

Copy link
Contributor

@Tibor17 Tibor17 Apr 17, 2022

Choose a reason for hiding this comment

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

This PR should not change the behavior. It should fix the value parallel=none and other previously unknown values for the same parameter. Structural changes are welcome as they do not change the behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can remove the implementation that bypasses the override of TestNG's default thread count, but it's unclear why the original code behaves this way. As it's currently implemented, specifying any form of parallel execution without explicitly specifying thread count will result in sequential execution.
The documentation here indicates that the default is 5, which would be true if the implementation in Surefire didn't impose a default value of 1.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should not change behavior in this PR. We are only aiming for the fix regarding configuration with the parameter parallel.
Speaking apart of thie PR, I would say that 5 is a kind of Bulgarian value in IT. The other frameworks do not use arbitrary values like TestNG does. The surefire providers should behave similar and so the surefire-junit47 has fallback value 0. The developer who wrote the documentation obviously had a problem with 5. Not sure if TestNG would have a problem with 0 (main Thread) and if 1 is a singleton non-main Thread. Feel free to check it out.

Copy link
Contributor Author

@sbabcoc sbabcoc Apr 20, 2022

Choose a reason for hiding this comment

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

@Tibor17 As you indicated, TestNG provides a default value of 5. As I pointed out, the Surefire documentation also specifies 5 as the default value. There is no potential for the thread count to have any value other than 5 unless an explicit assignment is made.
I'll remove the code that overrides the default implementation for the thread count parameter. I'll open another issue regarding the improper behavior of the existing implementation (forcing the thread count to 1 in the absence of an explicit assignment).

Copy link
Contributor Author

@sbabcoc sbabcoc Apr 20, 2022

Choose a reason for hiding this comment

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

Override of [threadCount] implementation removed. New issue opened here: https://issues.apache.org/jira/browse/SUREFIRE-2075

@Tibor17
Copy link
Contributor

Tibor17 commented Apr 23, 2022

@sbabcoc
@slawekjaranowski
I have forced CI to run the build. Let's wait for the result.

@Tibor17 Tibor17 merged commit cbf7df3 into apache:master Apr 23, 2022
@Tibor17
Copy link
Contributor

Tibor17 commented Apr 23, 2022

@sbabcoc Thx for contributing.

@sbabcoc sbabcoc deleted the pr/fix-testng-sequential branch April 25, 2022 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants