YARN-11969. Make catalog-webapp jasmine browser tests skippable independently of -DskipTests - #8621
Merged
Merged
Conversation
…endently of -DskipTests
The hadoop-yarn-applications-catalog-webapp module runs a jasmine-maven-plugin
test that drives a PhantomJS browser. In headless build environments PhantomJS
cannot start and the plugin fails with UnreachableBrowserException. Because this
is a plugin-execution failure rather than a Surefire test failure,
-Dmaven.test.failure.ignore=true does not suppress it and it fails the build.
The jasmine config previously hardcoded its skip flag to ${skipTests}, so the
only escape hatch was -DskipTests, which skips every test in the module.
Introduce a skipJasmineTests property defaulting to ${skipTests} and bind the
plugin's <skipJasmineTests> to it. Default behavior is unchanged; a headless
build can pass -DskipJasmineTests=true to skip only the browser-driven tests
while still compiling and covering the rest of the module. Build-config change
only; no production code affected.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
💔 -1 overall
This message was automatically generated. |
ferdelyi
marked this pull request as ready for review
July 23, 2026 12:46
slfan1989
approved these changes
Jul 26, 2026
slfan1989
left a comment
slfan1989
left a comment
Contributor
There was a problem hiding this comment.
The Java unit tests pass;
the reported failure occurs while starting the legacy PhantomJS binary on Ubuntu 24.04/OpenSSL3.
The Yetus command does not include -DskipJasmineTests=true, so this failure does not exercise the newly added option.
The property wiring looks correct and preserves the existing -DskipTests behavior. It would be helpful to include an end-to-end verification with:
mvn clean test -DskipJasmineTests=true
This is non-blocking from my side. +1.
…eTests takes effect Follow-up to the previous commit. Verifying that change with a real build (mvn test -DskipJasmineTests=true) showed the jasmine specs still ran, i.e. the new flag had no effect. Root cause: the jasmine-maven-plugin (2.1) already declares skipJasmineTests as @parameter(property="skipJasmineTests") and skips when skipTests, maven.test.skip or skipJasmineTests is true (isSkipTests() ORs the three). Providing an explicit <skipJasmineTests> element in the plugin <configuration> overrides that native property binding, and the same-named project property added in the previous commit shadowed the -DskipJasmineTests user property during interpolation, so the plugin never saw the flag. (mvn help:evaluate resolved the property to true, which was a misleading positive: it uses different precedence than plugin-config interpolation.) Fix: drop the explicit <skipJasmineTests> element and the added property entirely, and let the plugin's own bindings apply. -DskipTests continues to skip jasmine via isSkipTests(), while -DskipJasmineTests=true now skips only the browser-driven specs and leaves the module's other tests running. Verified with mvn test on the module: - -DskipJasmineTests=true -> "Skipping Jasmine Specs", build succeeds, surefire runs - -DskipTests -> "Skipping Jasmine Specs", build succeeds - (no flag) -> "Executing Jasmine Specs", build succeeds Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Thanks for the review, @slfan1989 — and especially for flagging that the Yetus run never exercised -DskipJasmineTests=true. I ran that end-to-end check locally as you suggested, and it turned out the flag didn't actually skip the jasmine specs, hence removed the explicit element (and the added property) entirely and rely on the plugin's own bindings. The independent skip works, -DskipTests still skips jasmine, default still runs. |
|
💔 -1 overall
This message was automatically generated. |
Member
|
should this be documented in |
pan3793
reviewed
Jul 29, 2026
| -DskipJasmineTests would be ignored. Omitting it lets | ||
| -DskipTests skip jasmine (as before) while -DskipJasmineTests=true | ||
| skips only the browser-driven specs (e.g. on a headless box | ||
| with no PhantomJS) without disabling the module's other tests. --> |
Member
There was a problem hiding this comment.
the words are too verbose, just say: Jasmine tests will be skipped if any one of -Dmaven.test.skip=true, -DskipTests, -DskipJasmineTests=true is set.
Contributor
Author
There was a problem hiding this comment.
Thank you for the review, @pan3793! Both addressed in the latest commit:
- Comment verbosity — trimmed the pom comment down to your suggested wording:
I kept a one-line "don't declare skipJasmineTests here" note, since a hardcoded element is exactly what was silently disabling the flag before — to guard against a re-add.
- BUILDING.txt — added a bullet under Tests options:
- Use -DskipJasmineTests=true to skip only the browser-driven jasmine tests in
hadoop-yarn-applications-catalog-webapp (e.g. on a headless machine with no
browser) while still running the module's other tests.
…smineTests Per review feedback from Cheng Pan (pan3793): - Shorten the verbose skipJasmineTests comment in the catalog-webapp pom to a single note that jasmine is skipped when any of -Dmaven.test.skip=true, -DskipTests or -DskipJasmineTests=true is set, and that no explicit skipJasmineTests element should be declared (it would override those flags). - Document -DskipJasmineTests=true in BUILDING.txt's "Tests options" section. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
pan3793
changed the title
pan3793
approved these changes
Jul 30, 2026
|
💔 -1 overall
This message was automatically generated. |
pan3793
approved these changes
Aug 2, 2026
pan3793
pushed a commit
that referenced
this pull request
Aug 2, 2026
…endently of -DskipTests (#8621) Authored-by: Ferenc Erdelyi <ferdelyi@cloudera.com> Reviewed-by: Shilun Fan <slfan1989@apache.org> Signed-off-by: Cheng Pan <chengpan@apache.org>
pan3793
pushed a commit
that referenced
this pull request
Aug 2, 2026
…endently of -DskipTests (#8621) Authored-by: Ferenc Erdelyi <ferdelyi@cloudera.com> Reviewed-by: Shilun Fan <slfan1989@apache.org> Signed-off-by: Cheng Pan <chengpan@apache.org>
Member
|
thanks, merged to trunk/3.5/3.4 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of PR
hadoop-yarn-applications-catalog-webappruns ajasmine-maven-plugintest thatdrives a PhantomJS browser. In headless build environments PhantomJS cannot start,
and the plugin fails with
UnreachableBrowserException. Because this is aplugin-execution failure rather than a Surefire test failure,
-Dmaven.test.failure.ignore=truedoes not suppress it and it fails the build.The module's jasmine config previously hardcoded its skip flag to
${skipTests},so the only way to avoid the browser test was
-DskipTests, which skips every testin the module.
This PR introduces a
skipJasmineTestsproperty (default${skipTests}) and bindsthe plugin's
<skipJasmineTests>to it. Default behavior is unchanged; a headlessbuild can pass
-DskipJasmineTests=trueto skip only the browser-driven tests whilestill compiling and covering the rest of the module.
Build-config change only; no production code affected.
How was this patch tested?
Single-file change to the catalog-webapp
pom.xml. Verified property resolutionwith
mvn help:evaluate:${skipTests}— same value the plugin received before.-DskipTests: resolves totrue(jasmine skipped, as before).-DskipJasmineTests=true: resolves totrue(jasmine skipped independently).For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?AI Tooling
If an AI tool was used:
where is the name of the AI tool used.
https://www.apache.org/legal/generative-tooling.html