Add remote read timeout as configurable parameter

[rodion-goritskov]

Proposed changes

I have quiet a small number of executors in my Selenium Grid (and in Browserstack too) and long running tests, so sometimes tests need to wait for the new browser session for more than 90 seconds.
In this pull-request I add new variable remoteReadTimeout to configuration with default value of 90000 ms, so I can configure read timeout from properties or command-line
Read timeout for local browsers is still 90 seconds and is not configurable (it seems reasonable).

Checklist

• Checkstyle and unit tests are passed locally with my changes by running gradlew check chrome_headless firefox_headless command
• I have added tests that prove my fix is effective or that my feature works. Are there any tests required for this code?
• I have added necessary documentation (if appropriate) - I have added JavaDoc to Configuration class. Are there any more changes required?

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

No Coverage information
0.0% Duplication

[asolntsev]

@rodion-goritskov We intentionally didn't introduce a config parameter because it's rarely needed.
Instead, you can just change this timeout as is:

@BeforeAll
void setupWebdriver() {
  HttpClientTimeouts.defaultReadTimeout = Duration.ofSeconds(180);
}

[rodion-goritskov]

@rodion-goritskov We intentionally didn't introduce a config parameter because it's rarely needed. Instead, you can just change this timeout as is:

@BeforeAll
void setupWebdriver() {
  HttpClientTimeouts.defaultReadTimeout = Duration.ofSeconds(180);
}

Yeah, I understand it could be overridden this way - but I thought about following cases:

1. So we could change it from command line (in runtime) without additional code in tests themselves
2. When manually creating SelenideDriver - so we can change it individually for each driver

[asolntsev]

@rodion-goritskov Well, yes, probably it makes sense.

NB! But isn't setting a LONG timeout a dirty workaround, not a real solution?
Basically it means that you have more parallel tests than available browsers. Why do you run so many parallel tests? It doesn't make sense because they are just wasting time while waiting for a browser.

Why don't you want to just configure proper number of Selenium Grid executors and parallel tests?

[rodion-goritskov]

NB!

@rodion-goritskov Well, yes, probably it makes sense.

NB! But isn't setting a LONG timeout a dirty workaround, not a real solution? Basically it means that you have more parallel tests than available browsers. Why do you run so many parallel tests? It doesn't make sense because they are just wasting time while waiting for a browser.

Why don't you want to just configure proper number of Selenium Grid executors and parallel tests?

Well, mostly there are three cases:

1. Different teams run their tests at the same time and accidentely quiet a long queue appears. We use Moon as a solution for running browsers, so our number of parallel threads is limited by license.
2. Our development k8s cluster used for Moon sometimes starts scaling nodes to start new browser, so it can be a couple of minutes.
3. We like to run some of our tests using real devices on Browserstack (especially iPhone/MacOS, as there are quiet a lot of problems there). We have only two threads available, so in case of all teams running tests at the same time we also have some long waits + these devices sometimes are not so fast, so tests could take a minute or two to complete.

These all are mostly "sometimes" cases, but with our current number of tests stability is much more important than speed.

P.S. We are now in process of Selenium Grid 4 adoption, but it will take some time

[asolntsev]

Are you sure this change really allows this?

When manually creating SelenideDriver - so we can change it individually for each driver

I am not sure. Look, instance of SelenideNettyClientFactory is created only once. It will use only the first timeout, even if you create hundreds of SelenideDriver with different timeouts.

P.S. What you are describing looks like a disaster. Why not use as many browsers as really needed? If you have multiple teams which all need to run tests - why not use as many Moon instances / Kubernetes notes / whatever else is needed to run tests comfortably? You are wasting time of your multiple teams - isn't it a huge waster of resources?

[BorisOsipov]

P.S. What you are describing looks like a disaster.

+1

[BorisOsipov]

It seems will be better get rid of this class at all and use default from global config.

[asolntsev]

We cannot do it because we still need to use HttpClientTimeouts.defaultReadTimeout when running browser locally (in class SelenideNettyClientFactory).

But yes, we could move field defaultReadTimeout from HttpClientTimeouts into SelenideNettyClientFactory and remove class HttpClientTimeouts.

[BorisOsipov]

LGTM. One minor question\comment