180145 – WebDriver: add support for importing and running selenium tests

RESOLVED FIXED 180145

WebDriver: add support for importing and running selenium tests

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

Summary WebDriver: add support for importing and running selenium tests

Carlos Garcia Campos

Reported 2017-11-29 08:53:56 PST

We currently import and run W3C tests, which are the best ones to ensure our implementation is spec compliant. However, the selenium API is what user will actually use in the end, so it's important to ensure that we don't break the selenium support.

Attachments
Patch (2.59 MB, patch) 2017-11-29 09:07 PST, Carlos Garcia Campos	no flags	Details Formatted Diff Diff
Updated patch (2.32 MB, patch) 2017-11-30 01:41 PST, Carlos Garcia Campos	bburg: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Carlos Garcia Campos

Comment 1 2017-11-29 09:07:28 PST

Created attachment 327861 [details] Patch

Blaze Burg

Comment 2 2017-11-29 13:31:54 PST

Comment on attachment 327861 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327861&action=review This is fine in principle. However, I don't want to import all the other junk in the Selenium repository, including docs, build system, and copies of all the client library sources, if you aren't going to actually use it. Can you change the importer to just import tests? > Tools/ChangeLog:12 Nit: selenium

Carlos Garcia Campos

Comment 3 2017-11-29 22:49:17 PST

(In reply to Brian Burg from comment #2) > Comment on attachment 327861 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=327861&action=review > > This is fine in principle. However, I don't want to import all the other > junk in the Selenium repository, including docs, build system, and copies of > all the client library sources, if you aren't going to actually use it. Can > you change the importer to just import tests? Ok, I'll try to reduce what we import. Note that we are not including a copy of all the client libs, only the python one that is needed to run the tests. > > Tools/ChangeLog:12 > > > Nit: selenium

Carlos Garcia Campos

Comment 4 2017-11-30 01:41:35 PST

Created attachment 327960 [details] Updated patch I've merged both import scripts into a single onw that can receive --w3c and --selenium arguments. The importer now ignores build files, documentation and the implementations an tests of other drivers.

Blaze Burg

Comment 5 2017-11-30 16:12:57 PST

Comment on attachment 327960 [details] 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? > 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. > 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. > 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? > Tools/Scripts/webkitpy/webdriver_tests/webdriver_selenium_executor.py:96 > + pytest.main(['--driver=%s' % self._name, Per above, this could be None. > WebDriverTests/imported/selenium/common/src/web/Page.aspx:1 > +<%@ Page Language="C#" AutoEventWireup="true" CodeFile="Page.aspx.cs" Inherits="Page" %> No reason to import this, as we don't run an ASP server =) > WebDriverTests/imported/selenium/common/src/web/Page.aspx.cs:3 > + Ditto but for .Net > WebDriverTests/imported/selenium/common/src/web/Redirect.aspx:6 > +<head runat="server"> Ditto > WebDriverTests/imported/selenium/common/src/web/Redirect.aspx.cs:4 > +{ Ditto > WebDriverTests/imported/selenium/common/src/web/Settings.StyleCop:3 > + <Analyzer AnalyzerId="Microsoft.StyleCop.CSharp.DocumentationRules"> Ditto > 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. > 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.

Carlos Garcia Campos

Comment 6 2017-12-01 01:32:02 PST

Comment on attachment 327960 [details] 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.

Carlos Garcia Campos

Comment 7 2017-12-01 03:32:07 PST

Committed r225390: <https://trac.webkit.org/changeset/225390>

Radar WebKit Bug Importer

Comment 8 2017-12-01 03:40:53 PST

<rdar://problem/35794680>

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component WebDriver

Assignee

Nobody

Reported

2017-11-29 08:53 PST

Modified

2017-12-01 03:40 PST History

CC List

5 users Show

URL

Keywords InRadar

Depends on

Blocks