[java] start the secure server only when needed in unit tests

[joerg1985]

User description

Description

This PR will add the @NeedsSecureServer to avoid starting the secure server in most of the unit tests.

Motivation and Context

Only ~5 classes need the secure server, all others do not need the secure server.
Starting the secure server is consuming alot of CPU to generate new SSL certificate.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

PR Type

enhancement, tests

---

Description

• Introduced a new annotation @NeedsSecureServer to conditionally start a secure server for specific test classes.
• Modified InProcessTestEnvironment and NettyAppServer to support conditional secure server initialization.
• Updated several test classes to use the new @NeedsSecureServer annotation.
• Enhanced JupiterTestBase to handle secure server requirements dynamically.
• Adjusted Bazel build configuration to include the new annotation.

---

Changes walkthrough 📝

	Relevant files
Tests	7 files CookieImplementationTest.java Add NeedsSecureServer annotation to CookieImplementationTest java/test/org/openqa/selenium/CookieImplementationTest.java Added @NeedsSecureServer annotation to the test class. +2/-0      PageLoadingTest.java Add NeedsSecureServer annotation to PageLoadingTest            java/test/org/openqa/selenium/PageLoadingTest.java Added @NeedsSecureServer annotation to the test class. +2/-0      NettyAppServerTest.java Update NettyAppServerTest for non-secure server initialization java/test/org/openqa/selenium/environment/webserver/NettyAppServerTest.java Updated test to create NettyAppServer without secure server. +1/-1      FederatedCredentialManagementTest.java Add NeedsSecureServer annotation to FederatedCredentialManagementTest java/test/org/openqa/selenium/federatedcredentialmanagement/FederatedCredentialManagementTest.java Added @NeedsSecureServer annotation to the test class. +2/-0      RemoteWebDriverBiDiTest.java Update RemoteWebDriverBiDiTest for non-secure server setup java/test/org/openqa/selenium/grid/router/RemoteWebDriverBiDiTest.java Updated server setup to initialize NettyAppServer without secure server. +1/-1      RemoteWebDriverDownloadTest.java Update RemoteWebDriverDownloadTest for non-secure server setup java/test/org/openqa/selenium/grid/router/RemoteWebDriverDownloadTest.java Updated setup to initialize NettyAppServer without secure server. +1/-1      CrossDomainTest.java Update CrossDomainTest for non-secure server setup              java/test/org/openqa/selenium/safari/CrossDomainTest.java Updated server setup to initialize NettyAppServer without secure server. +1/-1
Enhancement	5 files InProcessTestEnvironment.java Modify InProcessTestEnvironment to conditionally start secure server java/test/org/openqa/selenium/environment/InProcessTestEnvironment.java Modified constructor to accept a boolean for secure server. Updated main method to start with secure server. +3/-3      NettyAppServer.java Conditional secure server initialization in NettyAppServer java/test/org/openqa/selenium/environment/webserver/NettyAppServer.java Modified constructor to accept a boolean for secure server. Conditional initialization of secure server. +14/-12  JavaScriptTestSuite.java Update JavaScriptTestSuite for secure server and static lifecycle java/test/org/openqa/selenium/javascript/JavaScriptTestSuite.java Changed test environment setup to use secure server. Added static setup and teardown methods. +14/-9    JupiterTestBase.java Enhance JupiterTestBase to conditionally use secure server java/test/org/openqa/selenium/testing/JupiterTestBase.java Added logic to determine if secure server is needed. Restart server with secure option if necessary. +23/-1    NeedsSecureServer.java Introduce NeedsSecureServer annotation for test classes    java/test/org/openqa/selenium/testing/NeedsSecureServer.java Added new annotation @NeedsSecureServer for test classes. +30/-0
Configuration changes	1 files BUILD.bazel Add NeedsSecureServer to Bazel build configuration              java/test/org/openqa/selenium/testing/BUILD.bazel Added NeedsSecureServer.java to the annotations source list. +1/-0

---

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Potential Regression The main method now starts the environment with a secure server, which might affect existing tests or usage that rely on the previous behavior. Conditional Initialization The secure server is now conditionally initialized based on the secureServer parameter. Ensure this doesn't break existing tests that expect a secure server to always be available. Static Setup The test environment setup has been moved to a static method. Verify that this change doesn't cause issues with test isolation or parallel test execution. Dynamic Server Configuration The method now checks for the @NeedsSecureServer annotation and restarts the server if necessary. Ensure this doesn't introduce unexpected behavior or performance issues.

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Best practice	Add a default constructor to maintain backward compatibility Consider adding a default constructor that calls the new constructor with false as the argument to maintain backward compatibility with existing code that may be using the default constructor. java/test/org/openqa/selenium/environment/InProcessTestEnvironment.java [27-30] +public InProcessTestEnvironment() { + this(false); +} + public InProcessTestEnvironment(boolean secureServer) { appServer = new NettyAppServer(secureServer); appServer.start(); } Apply this suggestion Suggestion importance[1-10]: 7 Why: Adding a default constructor maintains backward compatibility with existing code that may be using the default constructor, while still allowing the new functionality with the boolean parameter.	7
Possible issue	Add a null check to prevent potential NullPointerExceptions Consider adding a null check for the secure variable before calling initValues(secure != null) to prevent potential NullPointerExceptions. java/test/org/openqa/selenium/environment/webserver/NettyAppServer.java [65-72] .onRetry( e -> { LOG.log( Level.WARNING, String.format("NettyAppServer retry #%s. ", e.getAttemptCount())); - initValues(secure != null); + initValues(secure != null && secure); }) Apply this suggestion Suggestion importance[1-10]: 6 Why: Adding a null check for the 'secure' variable prevents potential NullPointerExceptions, improving the robustness of the code.	6
Maintainability	Extract server restart logic into a separate method for better code organization Consider extracting the logic for checking and restarting the server with secure options into a separate method to improve readability and maintainability. java/test/org/openqa/selenium/testing/JupiterTestBase.java [70-81] -if (needsSecureServer) { - try { - appServer.whereIsSecure("/"); - } catch (IllegalStateException ex) { - // this should not happen with bazel, a new JVM is used for each class - // the annotation is on class level, so we should never see this - LOG.info("appServer is restarted with secureServer=true"); - environment.stop(); - environment = new InProcessTestEnvironment(true); +private void ensureSecureServerIfNeeded(boolean needsSecureServer) { + if (needsSecureServer) { + try { + appServer.whereIsSecure("/"); + } catch (IllegalStateException ex) { + LOG.info("appServer is restarted with secureServer=true"); + environment.stop(); + environment = new InProcessTestEnvironment(true); + } } } +// In the prepareEnvironment method: +ensureSecureServerIfNeeded(needsSecureServer); + Apply this suggestion Suggestion importance[1-10]: 5 Why: Extracting the server restart logic into a separate method improves code readability and maintainability, but the impact is moderate as it's mainly a refactoring suggestion.	5
Enhancement	Add Javadoc comments to improve annotation documentation Consider adding a brief Javadoc comment to explain the purpose and usage of the @NeedsSecureServer annotation for better documentation. java/test/org/openqa/selenium/testing/NeedsSecureServer.java [26-30] +/** + * Indicates that a test class requires a secure server for its tests. + * When applied to a test class, it ensures that a secure server is started + * before running the tests. + */ @Retention(RetentionPolicy.RUNTIME) @Target(ElementType.TYPE) public @interface NeedsSecureServer { + /** + * @return true if a secure server is needed, false otherwise + */ boolean value() default true; } Apply this suggestion Suggestion importance[1-10]: 4 Why: Adding Javadoc comments improves documentation and code clarity, but has a relatively low impact on functionality or performance.	4

💡 Need additional feedback ? start a PR chat

[pujagani]

LGTM. Thank you so much! This helps a lot. @joerg1985