Skip to content
This repository has been archived by the owner on Sep 21, 2021. It is now read-only.

No containers are started when desiredContainers = 0 and browserstack remote proxy is enabled #552

Closed
dennisgranath opened this issue May 3, 2018 · 13 comments
Labels

Comments

@dennisgranath
Copy link
Contributor

Please make sure that you provide enough information for us to help you with this issue. Thank you!

Zalenium Image Version(s):
3.11.0f

Expected Behavior -

By default it should start a container when os capability is not set.

Actual Behavior -

It runs the tests on browserstack random OS

@dspasojevic
Copy link
Contributor

dspasojevic commented May 4, 2018

@dennisgranath, I will take a look at this tonight if @diemol hasn't had a chance.

[EDIT - a word]

@dennisgranath
Copy link
Contributor Author

dennisgranath commented May 4, 2018

Maybe we should only call
TestSession newSession = super.getNewSession(desiredCapabilities);
if os is set to something that can't be handled by the containers?

Hmm. maybe not a good idea since container could be reused...

@diemol diemol added the bug label May 4, 2018
@diemol
Copy link
Contributor

diemol commented May 4, 2018

Ah ok, this was the ZaleniumCapabilityMatcher class job. But since we don't have the DockerSeleniumStarterProxy anymore then it is doing only half of the job. I think this is the class that needs to be improved.

@dspasojevic
Copy link
Contributor

@diemol would you like me to take a look at this?

@diemol
Copy link
Contributor

diemol commented May 4, 2018

That's be cool @dspasojevic, thanks!

What basically needs to happen is that the ZaleniumCapabilityMatcher matcher should return false if the capabilities can be fulfilled by docker-selenium.
In general, those capabilities are Chrome & Firefox on Linux. The tricky part is the browser version, which cannot be hardcoded since it can change with every elgalu/docker-selenium release. (That is why we had those chromeVersion and firefoxVersion variables in the DockerSelenium matcher).

@dspasojevic
Copy link
Contributor

Ah right. I guess that we don't need to consider a hypothetical situation where a different image is available for different browser versions - there is no capability to use more than one image.

I will take a look.

@dspasojevic
Copy link
Contributor

@diemol, I wonder if it would be a good idea for two proxy sets to be managed:

  1. containing the auto-started containers - these should be the first priority - but could be empty.
  2. containing manually registered containers (e.g. browserstack).

When capabilities are checked, set one would be checked first.

If:

  • auto-start capabilities are known (e.g. a container has been started before) and no match, go to second set.
  • if auto-start capabilities aren't known and it is unlikely to be supported (e.g. not CHROME or FIREFOX), go to the second set.
  • auto-start capabilities are known and no match, go to the second set.

Otherwise, try service by starting a new auto-started capability.

That way, we don't have to rely overly much on the correct sorting of proxies - we explicitly prefer to meet capabilities with the first proxy set.

WDYT?

@dspasojevic
Copy link
Contributor

This commit has a first cut of using two proxy sets to prioritise auto-starting over using a manual registered node.

I'm not sure how to check whether an auto-started node could be used.

@diemol
Copy link
Contributor

diemol commented May 4, 2018

I think it does not need to be that complex because the proxies have already methods to validate capabilities requested vs. the ones that can be fulfilled.

I would just keep it very simple since the matching can be done by each class done for that, perhaps splitting the proxies in two groups would solve the problem but not in the simplest way. I really think that it is a matter of modifying the matcher for the cloud proxies, and we can keep the change rather small.

@dspasojevic
Copy link
Contributor

Sure. I think that there is something to be said for explicitly preferring local nodes over cloud testing proxies, rather than relying on the proxy set sorting. If the logic for the potential capabilities of the local nodes could be expressed simply, I think that this would be a clean solution.

Happy to defer to you though - you know much more here than I do :)

@diemol
Copy link
Contributor

diemol commented May 5, 2018

Sounds good @dspasojevic, thanks for checking it though.
Talking about it here also helped me to find a more simple solution :)

@diemol diemol closed this as completed in 7fad876 May 6, 2018
@diemol
Copy link
Contributor

diemol commented May 6, 2018

Fixed, will release between today and tomorrow.

@dennisgranath
Copy link
Contributor Author

Awesome @diemol!! Thanks alot!!

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

No branches or pull requests

3 participants