Add support for https protocol for Remote drivers by platonoff-dev · Pull Request #272 · pytest-dev/pytest-selenium

platonoff-dev · 2021-09-02T16:47:42Z

isaulv · 2021-09-02T18:16:48Z

Did you test with selenium 4.0.0?

platonoff-dev · 2021-09-08T10:11:51Z

Yep. Looks like it work good with 4.0.0

dosas · 2021-09-20T18:49:25Z

    testdir.quick_qa("--driver", "Remote", file_test, passed=1)


+def test_https_host_port(testdir):


@platonoff-dev You added a check for https and no protocol but shouldn't you also test the case for http protocol?

I think test_default_host_port does the same...

pytest-selenium/testing/test_driver.py

Line 94 in a91b1f5

def test_default_host_port(testdir):

No I don't think so that is another case where the protocol is not explicitly given:

host expected url tested

https://... https://... yes

http://... http://... no

no.protocol http://no.protocol yes

localhost http://localhost yes

Yeah, I hurried with this a little

No problem, I think this can be solved with a simple parametrization: http, https

Writing this I realize that all of these tests (mentioned in the table) could probably be parametrized to one test

dosas · 2021-09-20T18:58:07Z

@@ -9,7 +9,9 @@


 def driver_kwargs(capabilities, firefox_profile, host, port, **kwargs):


Should we also fix this for the appium driver:

pytest-selenium/pytest_selenium/drivers/appium.py

Line 12 in a91b1f5

executor = "http://{0}:{1}/wd/hub".format(host, port)

?

Yeah, I forgot about appium 😅 I'll do that

dosas · 2021-09-20T19:06:39Z


 def driver_kwargs(capabilities, firefox_profile, host, port, **kwargs):
-    executor = "http://{0}:{1}/wd/hub".format(host, port)
+    executor = f"{host}:{port}/wd/hub"


@platonoff-dev I know that this is totally valid and working code but what about:

protocol = "" if re.search(r"^https?://", host) else "http://" executor = f"{protocol}{host}:{port}/wd/hub

I'm good with that. It's one of my problems 😅. I thought about that, but decided that my solution is more kind for new developers. They faster understand my version.

But your suggestion looks cleaner. I'll change that

I am not sure about the regexp though ... if it makes it cleaner or slower (due to import)

❯ python test_performance.py regexp: 0.009595889999999996 if: 0.004040118000000002

Almost two times slower...

Code that I ran:

import re import timeit def regexp_search(): host = "https://some.host.com" port = "4444" protocol = "" if re.search(r"^https?://", host) else "http://" executor = f"{protocol}{host}:{port}/wd/hub" def if_search(): host = "https://some.host.com" port = "4444" executor = f"{host}:{port}/wd/hub" if not executor.startswith("http://") and not executor.startswith("https://"): executor = "http://" + executor if __name__ == "__main__": print("regexp:", timeit.timeit(regexp_search, number=10000)) print("if:", timeit.timeit(if_search, number=10000))

Wow, thanks for testing that! I think that settles it then :D

BeyondEvil · 2021-10-18T20:28:44Z

Thanks for your contribution @platonoff-dev ! 🙏

Also, thanks to @dosas for the help! 🙏

Sorry it took me so long to look at it. :(

What about this:

def driver_kwargs(capabilities, firefox_profile, host, port, **kwargs):
    host = host if host.startswith("http") else f"http://{host}" 
    executor = "{0}:{1}/wd/hub".format(host, port)

    kwargs = {
        "command_executor": executor,
        "desired_capabilities": capabilities,
        "browser_profile": firefox_profile,
    }
    return kwargs

It's backwards compatible and if the user wants to use https, they simply provide the fully qualified url.

Thoughts?

platonoff-dev · 2021-10-18T20:49:40Z

I fixed it in my project with monkey patching and now it looks like this in my project :)

And I really forgot about this PR.

So, your suggestion looks great. But during fix in my project, I remembered a few new things:

Port is not required. https://some.hub.com/wd/hub is a valid URL. With current realization It can be handled with specifying 443 or 80 explicit. But it looks ugly.
Also, path can be different from /wd/hub

dosas · 2021-10-19T18:29:36Z

I do not know about the requirements, I just reviewed it based on what I thought the code should do and suggested missing tests based on possible combinations/scenarios.

dosas · 2021-11-15T20:01:24Z

@platonoff-dev Any news on this?

platonoff-dev · 2021-11-16T21:45:59Z

Oh, I completely forgot about this again( Yeah, I'll push required changes tomorrow.

platonoff-dev · 2021-11-27T13:49:43Z

@dosas Fixed all review remarks
It was a hard week and only now it turned out to be done)

dosas · 2021-12-01T19:19:49Z

+        "Remote",
+        "--selenium-host",
+        host,
+        "--selenium-port",


Since 4444 is the default there is no need to explicitly pass it here but fine for me.

platonoff-dev · 2022-03-23T11:49:54Z

Any changes I need to do?

BeyondEvil · 2022-03-24T13:18:57Z

Thanks for reminding me @platonoff-dev I don't think so at this point. I need to do some other work first, however. I apologise for the delay!

BeyondEvil · 2022-03-28T20:51:13Z

I tried to help out by rebasing the branch, but I couldn't figure out how to do it. 🤷‍♂️

@platonoff-dev

BeyondEvil · 2022-03-28T21:16:37Z

Once this PR is merged, i can continue with: #293

platonoff-dev · 2022-04-04T10:15:26Z

@BeyondEvil Ohh, I see it. But how can I do it? I have no write access, so I can't resolve it here. Just one option I can see is to rebase master into https-support and resolve conflict locally and then push. Is it ok?

BeyondEvil · 2022-04-04T10:32:43Z

@BeyondEvil Ohh, I see it. But how can I do it? I have no write access, so I can't resolve it here. Just one option I can see is to rebase master into https-support and resolve conflict locally and then push. Is it ok?

Yes, that's exactly how you do it. 😊 👍

$ git pull --rebase upstream master

(if you named the upstream "upstream" that is 😊 )

@platonoff-dev

BeyondEvil · 2022-04-04T12:09:57Z

I think something went wrong with the rebase @platonoff-dev 😬

platonoff-dev · 2022-04-04T12:13:46Z

Yeah 😬 Give me few minutes

platonoff-dev · 2022-04-04T12:27:18Z

@BeyondEvil looks like black in pre-commit is broken

BeyondEvil · 2022-04-04T12:31:22Z

Did you run pre-commit locally?

It looks like Flake8 complains about: testing/test_driver.py:149:1: F811 redefinition of unused 'test_host_protocol' from line 118

@platonoff-dev

platonoff-dev · 2022-04-04T12:33:19Z

This one is already fixed

BeyondEvil · 2022-04-04T12:34:30Z

This one is already fixed

Does pre-commit pass for you locally?

BeyondEvil · 2022-04-04T12:35:44Z

psf/black#2984

platonoff-dev · 2022-04-04T12:38:19Z

No, to commit fix I've used --no-verify. I wasn't sure if I should update black version here

BeyondEvil · 2022-04-04T12:39:23Z

No, to commit fix I've used --no-verify. I wasn't sure if I should update black version here

Sure, go ahead.

BeyondEvil

LGTM!

BeyondEvil · 2022-04-04T12:55:38Z

Nice! Almost there!

Can you squash/rebase all the commits to only relevant ones? (probably two, one for the https changes and one for the black bump). 🙏 🙇

Keep protocol if specified for apium too. Fix tests Add support for https protocol for Remote drivers Keep protocol if specified for apium too. Fix tests Remove duplicate of test

platonoff-dev · 2022-04-04T13:50:35Z

@BeyondEvil done

BeyondEvil

🚀

dosas reviewed Sep 20, 2021

View reviewed changes

dosas reviewed Dec 1, 2021

View reviewed changes

dosas previously approved these changes Dec 1, 2021

View reviewed changes

BeyondEvil mentioned this pull request Apr 4, 2022

Dependency conflict with pytest and selenium #294

Open

platonoff-dev dismissed dosas’s stale review via 73d78d8 April 4, 2022 12:07

BeyondEvil previously approved these changes Apr 4, 2022

View reviewed changes

Anatoliy Platonov and others added 2 commits April 4, 2022 17:42

Add support for https protocol for Remote drivers

5915063

Keep protocol if specified for apium too. Fix tests Add support for https protocol for Remote drivers Keep protocol if specified for apium too. Fix tests Remove duplicate of test

Update pre-commit black version

f26d8a4

platonoff-dev dismissed BeyondEvil’s stale review via f26d8a4 April 4, 2022 13:46

platonoff-dev requested a review from BeyondEvil April 4, 2022 13:48

BeyondEvil approved these changes Apr 4, 2022

View reviewed changes

BeyondEvil merged commit 5ba7a42 into pytest-dev:master Apr 4, 2022

		testdir.quick_qa("--driver", "Remote", file_test, passed=1)


		def test_https_host_port(testdir):

host	expected url	tested
https://...	https://...	yes
http://...	http://...	no
no.protocol	http://no.protocol	yes
localhost	http://localhost	yes

		@@ -9,7 +9,9 @@


		def driver_kwargs(capabilities, firefox_profile, host, port, **kwargs):

Conversation

platonoff-dev commented Sep 2, 2021

Uh oh!

isaulv commented Sep 2, 2021

Uh oh!

platonoff-dev commented Sep 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dosas Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dosas Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dosas Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BeyondEvil commented Oct 18, 2021

Uh oh!

platonoff-dev commented Oct 18, 2021

Uh oh!

dosas commented Oct 19, 2021

Uh oh!

dosas commented Nov 15, 2021

Uh oh!

platonoff-dev commented Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

platonoff-dev commented Nov 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

platonoff-dev commented Mar 23, 2022

Uh oh!

BeyondEvil commented Mar 24, 2022

Uh oh!

BeyondEvil commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BeyondEvil commented Mar 28, 2022

Uh oh!

platonoff-dev commented Apr 4, 2022

Uh oh!

BeyondEvil commented Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BeyondEvil commented Apr 4, 2022

Uh oh!

platonoff-dev commented Apr 4, 2022

Uh oh!

platonoff-dev commented Apr 4, 2022

Uh oh!

BeyondEvil commented Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

platonoff-dev commented Apr 4, 2022

dosas Sep 20, 2021 •

edited

Loading

dosas Sep 20, 2021 •

edited

Loading

dosas Sep 20, 2021 •

edited

Loading

platonoff-dev commented Nov 16, 2021 •

edited

Loading

BeyondEvil commented Mar 28, 2022 •

edited

Loading

BeyondEvil commented Apr 4, 2022 •

edited

Loading

BeyondEvil commented Apr 4, 2022 •

edited

Loading

BeyondEvil commented Apr 4, 2022 •

edited

Loading