Driver service timeout

[grigaman]

• By placing an X in the preceding checkbox, I verify that I have signed the Contributor License Agreement
  After thinking on comments of PR Using timeout from FirefoxBinary inside GeckoDriverService, XpiDriver… #6117 I think that adding configurable timeout into DriverService.Builder is much better solution, because, in this case, custom timeout can be set for any driver. Also because FirefoxBinary.setTimeout was not used at all I removed it.

---

This change is 

[grigaman]

I tried to add tests on *DriverService with mocking via mockito for testing passing timeout, but couldn't create correct partial mocks. This tests could be written with JMockit, because it supports mocking/verifying any calls (constructors, static methods). If this dependency could be added, I can write tests (which check that timeout is used in method waitUntilAvailable) for every DriverService.

[barancev]

Please don't add another mocking lib. If you need to mock a constructor you must be doing something wrong :) Or we have already written bad code that should be refactored for better testability.

[grigaman]

I didn't add any another mocking tool without approval. I am discussing if test is needed and which test I can add.
I can to add such tests for every driver(except SafariDriverService, because can't mock usingTechnologyPreview, which is called inside Builder constructor):

  @Test
  public void builderPassesTimeoutToDriverService() {
    File exe = new File("someFile");
    Duration defaultTimeout = Duration.ofSeconds(20);
    Duration customTimeout = Duration.ofSeconds(60);

    GeckoDriverService.Builder builderMock = spy(GeckoDriverService.Builder.class);
    doReturn(exe).when(builderMock).findDefaultExecutable();
    builderMock.build();

    verify(builderMock).createDriverService(any(), anyInt(), eq(defaultTimeout), any(), any());

    builderMock.withTimeout(customTimeout);
    builderMock.build();
    verify(builderMock).createDriverService(any(), anyInt(), eq(customTimeout), any(), any());
  }

But in this test there is no big sense, because it checks that the timeout is passed to Builder.createDriverService and can't check that set timeout in Builder is used by DriverService.
With JMockit test can check that timeout is used inside DriverService (with Mockito I think such test couldn't be written). Example of working test:

  @Test
  public void builderPassesTimeoutToDriverService2(@Mocked CommandLine cmdLine) throws Exception {
    File exe = new File("someFile");
    Duration defaultTimeout = Duration.ofSeconds(20);
    Duration customTimeout = Duration.ofSeconds(60);

    GeckoDriverService.Builder builder = new GeckoDriverService.Builder();
    new Expectations(builder, PortProber.class) {{
      builder.findDefaultExecutable(); result = exe;

      PortProber.waitForPortUp(anyInt, (int) defaultTimeout.toMillis(), MILLISECONDS);
      PortProber.waitForPortUp(anyInt, (int) customTimeout.toMillis(), MILLISECONDS);
    }};
    GeckoDriverService ds = builder.build();
    ds.start();

    new VerificationsInOrder() {{
      PortProber.waitForPortUp(anyInt, (int) defaultTimeout.toMillis(), MILLISECONDS);
    }};

    builder.withTimeout(customTimeout);
    ds = builder.build();
    ds.start();

    new VerificationsInOrder() {{
      PortProber.waitForPortUp(anyInt, (int) customTimeout.toMillis(), MILLISECONDS);
    }};
  }

[shs96c]

We have org.openqa.selenium.support.ui.Clock and org.openqa.selenium.support.ui.FakeClock if you need to control time in tests.

[shs96c]

A better choice would be java.time.Clock as that's part of the JRE and probably something we should look at using anyway.

[shs96c]

I've now migrated the code in Selenium to use java.time.Clock, btw, so this may need a rebase.

[grigaman]

I made a rebase on my branch, there is no need to change anything, because *DriverService don't use Clock

[grigaman]

It has been passed one and a half months. Do we process this pull request and integrate setting timeout of starting browser?

[grigaman]

I've committed tests which verify that timeout is passing to createDriverService for:
  ChromeDriverService.Builder
  EdgeDriverService.Builder
  GeckoDriverService.Builder
  XpiDriverService.Builder
  InternetExplorerDriverService.Builder
  OperaDriverService.Builder
SafariDriverService.Builder

[grigaman]

Do we process this pull request and integrate setting timeout of starting browser?