Add python client support for webextensions in firefox [rebased upon latest master and with fixed tests] #6463
Conversation
motin
force-pushed
the
add-python-client-support-for-webextensions-in-firefox
branch
2 times, most recently
from
c2d888e to
aebb2e6
Compare
motin
force-pushed
the
add-python-client-support-for-webextensions-in-firefox
branch
from
aebb2e6 to
9bef641
Compare
There was a problem hiding this comment.
I've left some comments that I'd like to see addressed, but I can confirm that this is installing a web extension when the id is specified. I don't want to block on this, so feel free to merge and release, and perhaps take care of the remaining items as followups.
| try: | ||
| id = manifest['applications']['gecko']['id'] | ||
| except KeyError: | ||
| id = manifest['name'].replace(" ", "") + "@" + manifest['version'] |
There was a problem hiding this comment.
Thank you for picking this up! I've looked into it more and discovered that whilst web extensions don't mandate an id, it appears that when installing extensions from the profile Firefox will verify that the name of the file/directory containing the extension matches the id of the add-on. This means that we effectively do require an id to be set in the manifest, and that we use it when writing to the profile directory. It may work by coincidence when the id matches the name@version format used here. It's worth testing with the other clients, but if this is the case then I believe we should require the id and raise an exception when it's not provided to avoid harder to debug issues with Firefox launching without the addon installed. Sorry for my earlier comment suggesting that we needed id to be optional, which was based on this documentation.
There was a problem hiding this comment.
True! Reported as #6471
| driver.quit() | ||
|
|
||
|
|
||
| def test_add_extension_web_extension_without_id(capabilities, webserver): |
There was a problem hiding this comment.
This test is failing for me.
There was a problem hiding this comment.
Is this due to the reasons you mentioned in https://github.com/SeleniumHQ/selenium/pull/6463/files#r221901851 or some other reason? The test seem to pass in travis
There was a problem hiding this comment.
The web extension doesn't appear to be copied to the profile path at all.
There was a problem hiding this comment.
As this is passing in Travis, let's go ahead and merge. It was the only thing that was making me uncertain about merging. It's failing locally for me, but I'm guessing it's related to the version of Firefox.
| @@ -171,3 +171,52 @@ def test_autodetect_proxy_is_set_in_profile(): | |||
|
|
|||
| profile.set_proxy(proxy) | |||
| assert profile.default_preferences["network.proxy.type"] == ProxyType.AUTODETECT['ff_value'] | |||
|
|
|||
|
|
|||
| def test_add_extension_web_extension_with_id(capabilities, webserver): | |||
There was a problem hiding this comment.
These tests are just for legacy Firefox, and we should also add coverage to py/test/selenium/webdriver/marionette
There was a problem hiding this comment.
It seems that many tests for legacy Firefox are missing for marionette. Should this be reported in general as a separate issue?
There was a problem hiding this comment.
I'd like to see more unit tests for the Selenium client that don't require a driver, for example we should be able to test several aspects of the FirefoxProfile class that aren't affected by the version of Firefox. Where there is a need for a driver test, we should at least make sure we have coverage in modern Firefox (marionette) as we will eventually be looking to drop support for legacy Firefox. If you're interested in contributing more, I'd love to get a coverage report for our unit tests and work on improving it - this will also give us confidence when we come to do larger scale refactoring.
|
Thanks for the contributions @motin and @sangaline. Please feel free to work on any of the follow ups, and to ping me for reviews or if you have any questions. |
This is a rebased and improved upon version of #5069, addressing the concerns raised by the reviewer.
Fixes the python part of #4093
Main contributors of this PR:
Xin the preceding checkbox, I verify that I have signed the Contributor License AgreementThis change is