[SUREFIRE-2075] Only set thread count if specified in configuration

[sbabcoc]

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.

• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004

• In any other case, please file an Apache Individual Contributor License Agreement.

[slawekjaranowski]

Please

• confirm by some of tests uint or IT
• add jira issue in commit message with square brackets

[Tibor17]

@sbabcoc
As I told you before, we will not accept this change. We spoke about it. The documentation in ASF says that 5 is used in TestNG and the developer originally said that this annoying value in TestNG can be changed because default value 5 does not make sense. I asked you to examine 0 and 1 because sequential order is default in all providers. 5 cannot be default value, this is nonsense. TestNG has this crazy value hardcoded.

[sbabcoc]

@Tibor17 The [threadCount] value is ignored if not running in parallel mode;
from [org.testng.TestRunner] in TestNG 7.3 : int threadCount = parallel ? xmlTest.getThreadCount() : 1;

(In the implementation of xmlTest.getThreadCount(), if [threadCount] is explicitly specified in XmlTest, this value is used; otherwise, the value is acquired from XmlSuite.)

In TestNG 5.1, tests are executed on the main thread when running in sequential mode.

I'm not clear why a default thread count of 5 doesn't make sense for parallel configurations. Setting the default value to 1 is what doesn't make sense to me, because this will produce sequential execution behavior in configurations that specify parallel execution unless a thread count value is also specified.

As it's currently implemented, the base Surefire TestNG configurator overrides the default thread count value established by the TestNG framework, altering the default parallel execution behavior. This default value imposed by TestNGMapConfigurator also contradicts the Surefire documentation.

With my revisions in place, the documented default thread count is used, and all of the unit tests and integration tests complete successfully.

[sbabcoc]

@Tibor17 Also, I was under the impression that the reason for rejecting this change previously was that it was out of scope for resolution of SUREFIRE-2064, not that the rationale for this change was in question.

[sbabcoc]

@slawekjaranowski I've added the JIRA to the commit message as requested, and all tests (unit + integration) run successfully.

[sbabcoc]

@Tibor17 With the revisions in this PR, the ASF TestNG implementation will exhibit the behavior you've described.

• With no value specified for the [parallel] setting, tests execute sequentially on a single thread.
• If any valid value is specified for the [parallel] setting (other than none), tests execute in parallel...
  • ... on 5 threads if the [threadCount] setting is unspecified (as indicated by the ASF documentation).
  • ... on [threadCount] threads if a valid integer value is specified for this setting.

Without the revisions in this PR, tests always execute sequentially unless valid values are provided for both [parallel] and [threadCount]. Specifying [parallel] by itself is insufficient to activate parallel execution behavior. This contradicts the ASF documentation and alters the execution behavior exhibited natively by TestNG or in other test harnesses (e.g. - Gradle).

The revisions in this PR resolve the issue of default [threadCount] value and always provide parallel execution behavior when [parallel] is specified. It also emits an actionable error message if an invalid value is specified for [threadCount], instead of the naked NumberFormatException that is currently produced.

[olamy]

LGTM and not sure how you could write a test for this?

[sbabcoc]

@olamy Thanks for the review! The existing unit tests cover "happy path" behavior. I suppose we could add an exception case test or two.

[slawekjaranowski]

Is there a test which set only parallel options without settings threadCount?

If not we can find test which set both parameters, and try with only one in copy of test.

[sbabcoc]

@slawekjaranowski I just added a test that sets the [parallel] option without setting [threadcount]. The existing test that provided the basis for this new implementation sets both [parallel] and [threadcount].