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.

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

Conversation

ErrorProne
Copy link

feat/enhancement: Variable hub port

Description

In some scenarios it is not possible or hard to set a fixed port for the selenium hub. For example if the hub is run dynamically in the CI pipeline through testcontainers.
This change allows to specify a different port from the default 4444 and thus, in the docker case, does not force to map the hub port fixed to 4444 on the host, which may lead to conflicting port mappings.

Fixes: #1657

Type of change

  • Bugfix
  • Feature
  • Enhancement

Checklist

  • I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.

Additional for Feature type of change

  • Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

In some scenarios it is not possible or hard to set a fixed port for the selenium hub. For example if the hub is run dynamically in the CI pipeline through testcontainers.
This change allows to specify a different port from the default 4444.
@CLAassistant
Copy link

CLAassistant commented Aug 4, 2023

CLA assistant check
All committers have signed the CLA.

String oldProperty = System.getProperty(HUB_PORT_PROPERTY);
try {
// 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

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

@mshabarov
Copy link
Contributor

@ErrorProne LGTM, could you please merge the latest main branch to yours? Validation fails with not-relevant errors for MultiSelectComboBox test wrapper.

@mshabarov
Copy link
Contributor

mshabarov commented Sep 1, 2023

Closed in favour of #1675.
I rebased it to a latest main to make the validation pass.
Thanks @ErrorProne for contribution!

@mshabarov mshabarov closed this Sep 1, 2023
@ErrorProne
Copy link
Author

@mshabarov Oh sorry, I'm currently hard caught up in work and didn't see your message earlier, but you've already rebased it. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PR request: Variable hub port
5 participants