[Webkit-unassigned] [Bug 180145] WebDriver: add support for importing and running selenium tests

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=180145

--- Comment #6 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 327960
  --> https://bugs.webkit.org/attachment.cgi?id=327960
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=327960&action=review

>> Tools/Scripts/import-webdriver-tests:91
>> +if '--w3c' in sys.argv or len(sys.argv) == 1:
> 
> It seems likely that we may want other CLI options in the future. Can you make this use argparse/optparse?

Ok.

>> Tools/Scripts/webkitpy/webdriver_tests/webdriver_driver.py:46
>> +    def selenium_name(self):
> 
> Please add a comment to explain the purpose of this. It appears to be used as the name to parameterize the test suite using pytest.

Yes, selenium adds --driver command line option to pytest that it uses to create the driver instance for the tests.

>> Tools/Scripts/webkitpy/webdriver_tests/webdriver_driver_wpe.py:45
>>  
> 
> Does WPE not need to run Selenium tests? Otherwise it needs a selenium name, I believe.

There's no WPE driver in selenium yet, so it's not possible to run selenium tests for now.

>> Tools/Scripts/webkitpy/webdriver_tests/webdriver_selenium_executor.py:32
>> +w3c_tools_dir = WebKitFinder(FileSystem()).path_from_webkit_base('WebDriverTests', 'imported', 'w3c', 'tools')
> 
> Would it make more sense to auto-install pytest rather than depending on the W3C drop? Or are the w3C tools dependent on their particular copy of pytest?

I'm reusing the recorder plugins of wpt, that I assumed depend on the pytest included in wpt tools. If those plugins are compatible with a newer pytest then, I think it's better to not import the wpt one and use the auto-installed for both. I'll investigate that possibility. But as long as we have a copy of pytest in the tree I think t's better to use it for selenium too to ensure same results format.

>> Tools/Scripts/webkitpy/webdriver_tests/webdriver_selenium_executor.py:96
>> +                pytest.main(['--driver=%s' % self._name,
> 
> Per above, this could be None.

No, collect_tests() and run_tests() return early in WebDriverTestRunnerSelenium when the name is None.

>> WebDriverTests/imported/selenium/py/conftest.py:113
>> +            browser_path = os.environ.get('WD_BROWSER_PATH')
> 
> The more elegant way to achieve this is to take the arguments via pytest_addoption. Let me know if you want to go that route; I recently changed safaridriver's test harness to stop using ENV like this. I think these options are generic enough that other drivers could use them.

hmm, note this is not a wk patch, this is upstream, I added recently to selenium for this and we discussed the possibility of making it generic for other drivers. I think we can discuss this upstream and then adopt here whatever we decide. See https://github.com/SeleniumHQ/selenium/pull/5121

>> WebDriverTests/imported/selenium/py/conftest.py:169
>> +    _path = '../buck-out/gen/java/server/src/org/openqa/grid/selenium/selenium.jar'
> 
> I hope none of the python tests use the server fixture. This hasn't been a problem in my experience.

No, this is not a problem, but I don't think it's worth patching this to remove this fixture.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20171201/72165d9a/attachment-0001.html>