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.

Sign up for GitHub

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

Add variable hub port capabilities #1658

Closed
wants to merge 7 commits into from
Closed

Add variable hub port capabilities #1658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b2fbae8
Add variable hubport capabilities
ErrorProne Aug 3, 2023
68dc070
Update vaadin-testbench-shared/src/main/java/com/vaadin/testbench/Par…
ErrorProne Aug 7, 2023
1572f60
Fixed javadoc
ErrorProne Aug 7, 2023
93b7942
Merge remote-tracking branch 'origin/variableHubPort' into variableHu…
ErrorProne Aug 7, 2023
cedc8e2
Respect that the hub hostname may vary in test environments
ErrorProne Aug 7, 2023
d48dabd
Reset hub hostname property
ErrorProne Aug 8, 2023
caf4205
Merge branch 'main' into variableHubPort
czp13 Aug 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...in-testbench-core-junit5/src/main/java/com/vaadin/testbench/browser/BrowserExtension.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ protected String getHubURL(Class<?> testClass) {
if (SauceLabsIntegration.isConfiguredForSauceLabs()) {
return SauceLabsIntegration.getHubUrl();
} else {
return "http://" + getHubHostname(testClass) + ":4444/wd/hub";
return "http://" + getHubHostname(testClass) + ":" + Parameters.getHubPort() + "/wd/hub";
}
}

Expand Down
32 changes: 32 additions & 0 deletions vaadin-testbench-core-junit5/src/test/java/com/vaadin/testbench/browser/BrowserHubTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
*/
package com.vaadin.testbench.browser;

import com.vaadin.testbench.Parameters;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

Expand All @@ -17,6 +18,7 @@
public class BrowserHubTest extends BrowserExtension {

private static final String HUB_HOSTNAME_PROPERTY = "com.vaadin.testbench.Parameters.hubHostname";
private static final String HUB_PORT_PROPERTY = "com.vaadin.testbench.Parameters.hubPort";

public BrowserHubTest() {
super(null);
Expand All @@ -43,4 +45,34 @@ public void hubFromAnnotationOrSystemProperty() {
}
}

@Test
public void hubPortFromDefaultValueOrParametersSetter() {
String oldPortProperty = System.getProperty(HUB_PORT_PROPERTY);
String oldHostnameProperty = System.getProperty(HUB_HOSTNAME_PROPERTY);

try {
// Reset the hub hostname property (removes the saucelab url in CI)
System.clearProperty(HUB_HOSTNAME_PROPERTY);

// Default must be the "official" 4444 port for backwards compatibility
Assertions.assertEquals(getExpectedHubUrl(4444), getHubURL(getClass()));
Copy link
Contributor

Choose a reason for hiding this comment

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

This assertions fail in the CI environment because getHubURL() returns the pre-configured sauce lab URL.
We should either change the assertion or add a workaround to temporarily skip the sauce lab integration

    private String sauceUser;

    @BeforeEach
    void temporaryDisableSauceLab() {
        if (SauceLabsIntegration.isConfiguredForSauceLabs()) {
            sauceUser = System.getProperty("sauce.user");
            System.clearProperty("sauce.user");
        }
    }

    @AfterEach
    void restoreSauceLab() {
        if (SauceLabsIntegration.isConfiguredForSauceLabs()
                && sauceUser != null) {
            System.setProperty("sauce.user", sauceUser);
        }
    }

Copy link
Author

Choose a reason for hiding this comment

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

Oh interesting, I thought that the getHubHostname call would fix it, but it returns the IP which does not match the actual hostname used. I'll change the approach for the assertion than.

Copy link
Author

@ErrorProne ErrorProne Aug 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm using the same mechanic like in the other test now

  // Reset the hub hostname property (removes the saucelab url in CI)
  System.clearProperty(HUB_HOSTNAME_PROPERTY);

I'm not a fan of the try finally which resets the properties tho. If it is fine for you I would move it from both tests into @BeforeEach and @AfterEach

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose that will not be enough. getHubUrl() will still return the SauceLab url.
But you can test it locally with mvn test -Dtest=BrowserHubTest -Dsauce.user=yyy -Dsauce.sauceAccessKey=xxx, from the vaadin-testbench-core-junit5 module.

I'm not a fan of the try finally which resets the properties tho. If it is fine for you I would move it from both tests into @BeforeEach and @AfterEach

It looks like a good improvement. Please go ahead


// Modified at runtime
Parameters.setHubPort(4445);
Assertions.assertEquals(getExpectedHubUrl(4445), getHubURL(getClass()));
} finally {
if (oldPortProperty != null) {
System.setProperty(HUB_PORT_PROPERTY, oldPortProperty);
}

if (oldHostnameProperty != null) {
System.setProperty(HUB_HOSTNAME_PROPERTY, oldHostnameProperty);
}
}
}

private String getExpectedHubUrl(int port) {
return "http://" + getHubHostname(getClass()) + ":" + port + "/wd/hub";
}

}
24 changes: 24 additions & 0 deletions vaadin-testbench-shared/src/main/java/com/vaadin/testbench/Parameters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ public class Parameters {
private static String testbenchGridBrowsers;
private static boolean headless;
private static int readTimeout;
private static int hubPort;
static {
isDebug = getSystemPropertyBoolean("debug", false);

Expand Down Expand Up @@ -64,6 +65,7 @@ public class Parameters {
headless = getSystemPropertyBoolean("headless", false);
readTimeout = getSystemPropertyInt("readTimeout",
(int) ClientConfig.defaultConfig().readTimeout().toSeconds());
hubPort = getSystemPropertyInt("hubPort", 4444);
}

/**
Expand Down Expand Up @@ -319,6 +321,28 @@ public static String getHubHostname() {
return getSystemPropertyString("hubHostname", null);
}

/**
* Gets the port of the hub to run tests on.
* <p>
* Allows to get a variable port for the selenium hub at runtime and can be modified
* from either via {@link #setHubPort(int)} or by setting the system property
* {@code com.vaadin.testbench.Parameters.hubPort}.
*
* @return the port of the hub, defaults to 4444 if nothing is set
*/
public static int getHubPort() {
return hubPort;
}

/**
* Changes the port on which the selenium hub is reachable.
*
* @param port The new port to use
*/
public static void setHubPort(int port) {
hubPort = port;
}

/**
* Gets the name of the browser to use for a local test.
* <p>
Expand Down