Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Code Health] Fix ABC wpt.browser.Browser #9349

Merged
merged 1 commit into from Feb 28, 2018

Conversation

@Hexcles
Copy link
Member

Hexcles commented Feb 1, 2018

  • Make sure methods in subclasses have the same interface as the
    abstract methods in the base class.
  • Move docstrings to the base class.
  • Declare two more abstract methods: find_binary & find_webdriver,
    because multiple (though not all) subclasses already defined these
    methods, and the two methods are called externally (by wpt.run). They
    fall into the same category as version().

This change is Reviewable

@Hexcles Hexcles requested review from jgraham and foolip Feb 1, 2018

@wpt-pr-bot wpt-pr-bot added the infra label Feb 1, 2018

@wpt-pr-bot wpt-pr-bot requested a review from gsnedders Feb 1, 2018

@jgraham

jgraham approved these changes Feb 1, 2018

@Hexcles Hexcles force-pushed the chrome-android branch 2 times, most recently from b73f8e1 to 5a8d6a0 Feb 1, 2018

@Hexcles Hexcles force-pushed the fix-browser-abc branch 2 times, most recently from 235e7d2 to 8dd2019 Feb 1, 2018

return NotImplemented

@abstractmethod
def install_webdriver(self):
def install_webdriver(self, dest=None):
"""Install the Webdriver implementation for this browser."""

This comment has been minimized.

Copy link
@foolip

foolip Feb 2, 2018

Contributor

WebDriver capitalization here and elsewhere

This comment has been minimized.

Copy link
@Hexcles

Hexcles Feb 2, 2018

Author Member

Thanks. I'll grep and fix.

@@ -239,11 +252,14 @@ def platform_string(self):

return "%s%s" % (platform, bits)

def find_binary(self):
# ChromeDriver can find browser binary itself.

This comment has been minimized.

Copy link
@foolip

foolip Feb 2, 2018

Contributor

I take it this is supposed to be a no-op change and it probably is, but changing this kind of code does me nervous about ways in which wpt.fyi running might run. Are there any tests at all for this stuff? @gsnedders?

This comment has been minimized.

Copy link
@Hexcles

Hexcles Feb 2, 2018

Author Member

An actual no-op would be return None. Throwing an exception would prevent future accidental use of these methods (these methods are not called currently).

I don't think there are any tests specifically for the stuff. But we do have integration tests for major products, don't we?

@@ -294,6 +309,9 @@ class ChromeAndroid(Browser):
def install(self, dest=None):
raise NotImplementedError

def find_binary(self):
raise NotImplementedError

This comment has been minimized.

Copy link
@foolip

foolip Feb 2, 2018

Contributor

A comment for this case as well?

@@ -396,14 +417,16 @@ class Edge(Browser):
def install(self, dest=None):
raise NotImplementedError

def find_binary(self):
raise NotImplementedError

This comment has been minimized.

Copy link
@foolip

foolip Feb 2, 2018

Contributor

Comment? Or maybe hoist all the comments into one place and remove them above instead.

This comment has been minimized.

Copy link
@Hexcles

Hexcles Feb 2, 2018

Author Member

Let me put some texts in the base class.

@@ -223,6 +223,7 @@ def setup_kwargs(self, kwargs):
else:
raise WptrunError("Unable to locate or install chromedriver binary")


This comment has been minimized.

Copy link
@foolip

foolip Feb 2, 2018

Contributor

Accidental change? Just a whitespace change in a file looks weird.

This comment has been minimized.

Copy link
@Hexcles

Hexcles Feb 2, 2018

Author Member

It's actually intentional, but only for styles... We should have two blank lines between top-level classes/functions according to PEP8. And this is the only offence in this file which I happened to spot.

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Feb 2, 2018

What is ABC?

@Hexcles
Copy link
Member Author

Hexcles left a comment

ABC is a Pythonic jargon: Abstract Base Class

@@ -223,6 +223,7 @@ def setup_kwargs(self, kwargs):
else:
raise WptrunError("Unable to locate or install chromedriver binary")


This comment has been minimized.

Copy link
@Hexcles

Hexcles Feb 2, 2018

Author Member

It's actually intentional, but only for styles... We should have two blank lines between top-level classes/functions according to PEP8. And this is the only offence in this file which I happened to spot.

return NotImplemented

@abstractmethod
def install_webdriver(self):
def install_webdriver(self, dest=None):
"""Install the Webdriver implementation for this browser."""

This comment has been minimized.

Copy link
@Hexcles

Hexcles Feb 2, 2018

Author Member

Thanks. I'll grep and fix.

@@ -396,14 +417,16 @@ class Edge(Browser):
def install(self, dest=None):
raise NotImplementedError

def find_binary(self):
raise NotImplementedError

This comment has been minimized.

Copy link
@Hexcles

Hexcles Feb 2, 2018

Author Member

Let me put some texts in the base class.

@@ -239,11 +252,14 @@ def platform_string(self):

return "%s%s" % (platform, bits)

def find_binary(self):
# ChromeDriver can find browser binary itself.

This comment has been minimized.

Copy link
@Hexcles

Hexcles Feb 2, 2018

Author Member

An actual no-op would be return None. Throwing an exception would prevent future accidental use of these methods (these methods are not called currently).

I don't think there are any tests specifically for the stuff. But we do have integration tests for major products, don't we?

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Feb 8, 2018

Build PASSED

Started: 2018-02-28 19:29:12
Finished: 2018-02-28 19:43:13

View more information about this build on:

return NotImplemented

@abstractmethod
def requirements(self):
"""Name of the browser-specific wptrunner requirements file"""
return NotImplemented

# TODO(robertma): This method doesn't seem to be called anywhere.

This comment has been minimized.

Copy link
@jgraham

jgraham Feb 8, 2018

Contributor

This was used to work around a Chrome issue, but I think it's dead code now.

This comment has been minimized.

Copy link
@Hexcles

Hexcles Feb 28, 2018

Author Member

Dead code has been removed.

@@ -131,6 +143,12 @@ def find_certutil(self):
def find_webdriver(self):
return find_executable("geckodriver")

def _get_executive_path(self, path, exe):

This comment has been minimized.

Copy link
@jgraham

jgraham Feb 8, 2018

Contributor

I'm not sure that making this a method rather than a function is an improvement. It's not like what it does is specific to this particular class.

This comment has been minimized.

Copy link
@Hexcles

Hexcles Feb 28, 2018

Author Member

This (along with its caller) has been removed.

@gsnedders

This comment has been minimized.

Copy link
Contributor

gsnedders commented Feb 28, 2018

@Hexcles this has merge errors and review comments, would be nice to land this

[Code Health] Fix ABC wpt.browser.Browser
* Make sure methods in subclasses have the same interface as the
  abstract methods in the base class.
* Move docstrings to the base class.
* Declare two more abstract methods: find_binary & find_webdriver,
  because multiple (though not all) subclasses already defined these
  methods, and the two methods are called externally (by wpt.run). They
  fall into the same category as version().

@Hexcles Hexcles force-pushed the fix-browser-abc branch from 8407951 to 2396d5b Feb 28, 2018

@Hexcles Hexcles merged commit 3370b65 into master Feb 28, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Hexcles

This comment has been minimized.

Copy link
Member Author

Hexcles commented Feb 28, 2018

@gsnedders done.

@Hexcles Hexcles deleted the fix-browser-abc branch Feb 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.