From 10db613788260242da7f150bb479c53a9a167bf2 Mon Sep 17 00:00:00 2001 From: Mohammad Saddam Hossain Date: Tue, 28 Jul 2026 07:24:41 +0530 Subject: [PATCH] fix: validate execution.parallel against TestNG's own modes (#35) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug 2 — ExecutionValidator threw "Parallel execution configuration missing" for a value that was present but not on its hand-written allowlist, sending the user hunting for config that was not absent. The allowlist duplicated an enum that already exists, and the two had already drifted: the value is handed straight to XmlSuite.setParallel() downstream, and nothing in the framework branches on methods vs classes — only on whether the mode is "none". So `tests` and `instances` were rejected despite being legitimate TestNG modes that would have worked. Validation now delegates to XmlSuite.ParallelMode, so anything TestNG accepts, Selenium Boot accepts. An unrecognised value names both the rejected value and the valid set, read off the enum rather than a list that can drift again. A null or blank value keeps the old "missing" message, which was always accurate for that case. Also aligned SuiteExecutionListener's own valueOf() to Locale.ROOT. Without it the validator and the consumer normalise differently, and a value that passes validation can still blow up at suite start — the same class of drift this change exists to remove. Bug 3 — parallel.md claimed maxActiveSessions "cannot exceed threadCount". Nothing enforces that; getOrInitSemaphore() sizes the semaphore from maxActiveSessions alone. The same file contradicted the claim twelve lines later. Docs list the two newly-accepted modes, gated with a version note: the docs site deploys from master, and advertising `tests` as valid while v3.2.0 on Central still rejects it would recreate the original bug with the sign flipped. Drop the note when the next release ships. New ExecutionValidatorTest covers all five modes, case-insensitivity, both message defects as regressions, and the threadCount bounds that had no coverage. 494 tests pass (was 480); Docusaurus build green. Fixes #35 Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 2 +- docs-site/docs/configuration.md | 10 +- docs-site/docs/guides/parallel.md | 13 +- .../execution/ExecutionValidator.java | 27 +++- .../listeners/SuiteExecutionListener.java | 2 +- .../unit/ExecutionValidatorTest.java | 126 ++++++++++++++++++ 6 files changed, 170 insertions(+), 10 deletions(-) create mode 100644 src/test/java/com/seleniumboot/unit/ExecutionValidatorTest.java diff --git a/CLAUDE.md b/CLAUDE.md index 7389188..2e7fd3e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -95,7 +95,7 @@ Consumer projects configure via `selenium-boot.yml` at the project root: execution: mode: local | remote baseUrl: https://example.com - parallel: none | methods | classes + parallel: none | methods | classes | tests | instances threadCount: 4 browser: diff --git a/docs-site/docs/configuration.md b/docs-site/docs/configuration.md index f40fe60..0145743 100644 --- a/docs-site/docs/configuration.md +++ b/docs-site/docs/configuration.md @@ -45,7 +45,7 @@ execution: baseUrl: https://your-app.com gridUrl: http://localhost:4444 # only for remote mode - parallel: none # none | methods | classes + parallel: none # none | methods | classes | tests | instances threadCount: 1 # ignored when parallel: none maxActiveSessions: 5 # max concurrent browser instances (semaphore) @@ -181,13 +181,19 @@ Use `per-suite` when your suite has many sequential tests and browser startup ti ## Execution ### `parallel` -Maps directly to TestNG parallel execution mode. Thread count is set via `threadCount`. Selenium Boot validates this value at suite bootstrap and rejects anything outside the three below — TestNG's `tests` mode is not currently accepted. +Maps directly to TestNG parallel execution mode. Thread count is set via `threadCount`. Selenium Boot validates this value at suite bootstrap against TestNG's own set of modes; an unrecognised value fails immediately with a message naming both the rejected value and the accepted ones. | Value | Behaviour | |---|---| | `none` | Sequential execution (default) | | `methods` | Each `@Test` method runs in its own thread | | `classes` | Each test class runs in its own thread | +| `tests` | Each `` in the suite XML runs in its own thread | +| `instances` | Each test class instance runs in its own thread | + +:::note Version +`tests` and `instances` are accepted from the next release onwards. **v3.2.0, the current release on Maven Central, accepts only `none`, `methods` and `classes`.** +::: ### `maxActiveSessions` Maximum concurrent browser instances. Tests wait (up to 30s) for a slot rather than failing immediately. Prevents resource exhaustion in parallel runs. diff --git a/docs-site/docs/guides/parallel.md b/docs-site/docs/guides/parallel.md index da768af..85b8363 100644 --- a/docs-site/docs/guides/parallel.md +++ b/docs-site/docs/guides/parallel.md @@ -17,9 +17,9 @@ Selenium Boot supports parallel test execution out of the box. Configure the thr ```yaml title="selenium-boot.yml" execution: mode: local - parallel: methods # none (default) | methods | classes + parallel: methods # none (default) | methods | classes | tests | instances threadCount: 4 # number of concurrent browser sessions - maxActiveSessions: 4 # semaphore cap — cannot exceed threadCount + maxActiveSessions: 4 # semaphore cap on concurrent browsers browser: name: chrome @@ -85,12 +85,19 @@ You do not need to do anything special in your tests. `getDriver()` always retur ## Parallel modes -TestNG supports several parallel modes. Selenium Boot currently validates `execution.parallel` against `none`, `methods`, and `classes` — any other value (including TestNG's `tests` mode) is rejected at suite bootstrap, before any test runs. +`execution.parallel` is validated at suite bootstrap, before any test runs, against TestNG's own set of parallel modes. An unrecognised value fails immediately with a message naming both the rejected value and the accepted ones. | Mode | Description | Recommended | |---|---|---| +| `none` | Sequential execution (default) | | | `methods` | Each test method runs in its own thread | Best general choice | | `classes` | Each test class runs in a thread | Use when tests within a class must be sequential | +| `tests` | Each `` in the suite XML runs in a thread | Use to isolate suite-level groupings | +| `instances` | Each test class instance runs in a thread | Rarely needed — factory-driven suites | + +:::note Version +`tests` and `instances` are accepted from the next release onwards. **v3.2.0, the current release on Maven Central, accepts only `none`, `methods` and `classes`** and rejects the other two at bootstrap. +::: --- diff --git a/src/main/java/com/seleniumboot/execution/ExecutionValidator.java b/src/main/java/com/seleniumboot/execution/ExecutionValidator.java index 7d02020..1113035 100644 --- a/src/main/java/com/seleniumboot/execution/ExecutionValidator.java +++ b/src/main/java/com/seleniumboot/execution/ExecutionValidator.java @@ -1,6 +1,11 @@ package com.seleniumboot.execution; import com.seleniumboot.config.SeleniumBootConfig; +import org.testng.xml.XmlSuite; + +import java.util.Arrays; +import java.util.Locale; +import java.util.stream.Collectors; public class ExecutionValidator { private ExecutionValidator() {} @@ -11,11 +16,20 @@ public static void validate(SeleniumBootConfig.Execution execution) { } String parallel = execution.getParallel(); - if (!"none".equalsIgnoreCase(parallel) - && !"methods".equalsIgnoreCase(parallel) - && !"classes".equalsIgnoreCase(parallel)) { + if (parallel == null || parallel.trim().isEmpty()) { throw new IllegalStateException("Parallel execution configuration missing"); } + try { + // Delegate to TestNG's own enum rather than a hand-written allowlist — the + // value is passed straight to XmlSuite.setParallel() downstream, so anything + // TestNG accepts, Selenium Boot accepts. + XmlSuite.ParallelMode.valueOf(parallel.toUpperCase(Locale.ROOT)); + } catch (IllegalArgumentException e) { + throw new IllegalStateException( + "Invalid execution.parallel value '" + parallel + + "' — valid values are " + validParallelModes() + ); + } int threads = execution.getThreadCount(); if (threads < 1) { @@ -29,4 +43,11 @@ public static void validate(SeleniumBootConfig.Execution execution) { ); } } + + private static String validParallelModes() { + return Arrays.stream(XmlSuite.ParallelMode.values()) + .map(mode -> mode.name().toLowerCase(Locale.ROOT)) + .sorted() + .collect(Collectors.joining(", ")); + } } diff --git a/src/main/java/com/seleniumboot/listeners/SuiteExecutionListener.java b/src/main/java/com/seleniumboot/listeners/SuiteExecutionListener.java index 6302b73..6d86c23 100644 --- a/src/main/java/com/seleniumboot/listeners/SuiteExecutionListener.java +++ b/src/main/java/com/seleniumboot/listeners/SuiteExecutionListener.java @@ -44,7 +44,7 @@ public void onStart(ISuite suite) { XmlSuite.ParallelMode mode = XmlSuite.ParallelMode.valueOf( - execution.getParallel().toUpperCase() + execution.getParallel().toUpperCase(java.util.Locale.ROOT) ); xmlSuite.setParallel(mode); diff --git a/src/test/java/com/seleniumboot/unit/ExecutionValidatorTest.java b/src/test/java/com/seleniumboot/unit/ExecutionValidatorTest.java new file mode 100644 index 0000000..8da1b91 --- /dev/null +++ b/src/test/java/com/seleniumboot/unit/ExecutionValidatorTest.java @@ -0,0 +1,126 @@ +package com.seleniumboot.unit; + +import com.seleniumboot.config.SeleniumBootConfig; +import com.seleniumboot.execution.ExecutionValidator; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Test; + +import static org.testng.Assert.*; + +/** + * Unit tests for {@link ExecutionValidator}. + */ +public class ExecutionValidatorTest { + + private SeleniumBootConfig.Execution execution(String parallel, int threadCount) { + SeleniumBootConfig.Execution execution = new SeleniumBootConfig.Execution(); + execution.setParallel(parallel); + execution.setThreadCount(threadCount); + return execution; + } + + private String messageOf(SeleniumBootConfig.Execution execution) { + try { + ExecutionValidator.validate(execution); + } catch (IllegalStateException e) { + return e.getMessage(); + } + fail("expected validate() to throw"); + return null; + } + + // ---------------------------------------------------------- + // parallel — accepted values + // ---------------------------------------------------------- + + @DataProvider(name = "validParallelModes") + public Object[][] validParallelModes() { + return new Object[][]{{"none"}, {"methods"}, {"classes"}, {"tests"}, {"instances"}}; + } + + /** + * Every mode TestNG's own XmlSuite.ParallelMode accepts is accepted here — the value + * is handed straight to XmlSuite.setParallel() downstream. "tests" and "instances" + * were rejected before the fix for issue #35. + */ + @Test(dataProvider = "validParallelModes") + public void validate_acceptsEveryTestNGParallelMode(String mode) { + ExecutionValidator.validate(execution(mode, 1)); + } + + @Test + public void validate_parallelIsCaseInsensitive() { + ExecutionValidator.validate(execution("METHODS", 1)); + ExecutionValidator.validate(execution("Tests", 1)); + } + + /** + * The validator must accept exactly what SuiteExecutionListener can later parse — + * both uppercase via Locale.ROOT — or the two drift and a "valid" value blows up at + * suite start instead of at validation. + */ + @Test + public void validate_untrimmedParallel_isRejectedLikeDownstream() { + String message = messageOf(execution(" classes ", 1)); + assertTrue(message.contains("classes"), "was: " + message); + } + + // ---------------------------------------------------------- + // parallel — rejected values + // ---------------------------------------------------------- + + /** + * Regression test for issue #35, bug 2: an unsupported-but-present value used to + * report "Parallel execution configuration missing", sending the user hunting for + * config that was not absent. The message must name the offending value. + */ + @Test + public void validate_unknownParallel_messageNamesRejectedValue() { + String message = messageOf(execution("threads", 1)); + assertTrue(message.contains("threads"), + "message should name the rejected value, was: " + message); + assertFalse(message.contains("missing"), + "a present-but-invalid value is not a missing one, was: " + message); + } + + @Test + public void validate_unknownParallel_messageListsValidValues() { + String message = messageOf(execution("threads", 1)); + for (String valid : new String[]{"none", "methods", "classes", "tests", "instances"}) { + assertTrue(message.contains(valid), + "message should list '" + valid + "', was: " + message); + } + } + + @Test + public void validate_nullParallel_reportsMissing() { + assertEquals(messageOf(execution(null, 1)), "Parallel execution configuration missing"); + } + + @Test + public void validate_blankParallel_reportsMissing() { + assertEquals(messageOf(execution(" ", 1)), "Parallel execution configuration missing"); + } + + // ---------------------------------------------------------- + // execution block / threadCount + // ---------------------------------------------------------- + + @Test + public void validate_nullExecution_reportsMissing() { + assertEquals(messageOf(null), "Execution configuration missing"); + } + + @Test + public void validate_threadCountBelowOne_isRejected() { + assertEquals(messageOf(execution("methods", 0)), "Thread count must be >= 1"); + } + + @Test + public void validate_threadCountAboveSafeLimit_isRejected() { + int overLimit = Runtime.getRuntime().availableProcessors() * 2 + 1; + String message = messageOf(execution("methods", overLimit)); + assertTrue(message.contains("exceeds safe limit"), "was: " + message); + assertTrue(message.contains(String.valueOf(overLimit)), "was: " + message); + } +}