Run mypy for tools/wptrunner/ (but still allow untyped defs) #28942

foolip · 2021-05-10T14:47:55Z

Make ConfigDict a Dict[str, Any] since the keys come from
parser.options() which are strings in typeshed:
https://github.com/python/typeshed/blob/7a9b4f93baa03fff3ef7f319988cea2cbb6e2135/stdlib/configparser.pyi#L105

Also s/SafeConfigParser/ConfigParser/ since SafeConfigParser is now a
legacy alias not mentioned in Python 3 documentation.

Part of #28833.

tools/wptrunner/wptrunner/executors/base.py

foolip · 2021-05-10T14:51:47Z

tools/wptrunner/wptrunner/browsers/edge.py

@@ -65,7 +65,6 @@ def env_options():


 class EdgeBrowser(Browser):
-    used_ports = set()


This was unused.

foolip · 2021-05-10T14:51:53Z

tools/wptrunner/wptrunner/browsers/ie.py

@@ -41,8 +41,6 @@ def env_options():
    return {"supports_debugger": False}

 class InternetExplorerBrowser(Browser):
-    used_ports = set()


This was unused.

tools/wptrunner/wptrunner/executors/base.py

tools/wptrunner/wptrunner/manifestupdate.py

foolip · 2021-05-17T14:29:09Z

@jgraham this isn't 100% done, but it's pretty big and was the hardest mypy PR so far, so sending it for review instead of continuing to polish. If there are parts you think can be split out for clarity, I'm happy to do that.

foolip · 2021-05-17T16:42:34Z

OK, I've made some additional changes so that I'm now happy enough with this myself. Ready for proper review.

jgraham

So generally I think I"m happy with this but I've also bitrotted you, so it's going to need an update.

Make ConfigDict a Dict[str, Any] since the keys come from parser.options() which are strings in typeshed: https://github.com/python/typeshed/blob/7a9b4f93baa03fff3ef7f319988cea2cbb6e2135/stdlib/configparser.pyi#L105 Also s/SafeConfigParser/ConfigParser/ since SafeConfigParser is now a legacy alias not mentioned in Python 3 documentation. Part of #28833.

foolip · 2021-05-24T13:36:24Z

tools/wptrunner/wptrunner/executors/base.py

@@ -710,7 +720,7 @@ def after_connect(self):


 class WdspecProtocol(Protocol):
-    server_cls = None
+    server_cls = None  # type: ClassVar[Optional[Type[WebDriverServer]]]


Optional is needed here because of this code added a few days ago:

wpt/tools/wptrunner/wptrunner/executors/executormarionette.py

Lines 1100 to 1101 in c21b0b8

class GeckoDriverProtocol(WdspecProtocol):

server_cls = None # To avoid circular imports we set this at runtime

I didn't add a comment referring to GeckoDriverProtocol because it seems guaranteed to go stale, but that's the reason if someone comes looking at this PR.

Yeah, this is a design problem. #29089 will address that once it's ready.

foolip · 2021-05-24T13:40:18Z

@jgraham rebased to adapt to recent changes, which was mostly throwing away changes no longer needed. I've looked over the full set of changes and it's hard to be confident every single one of them is now needed, but it's not very far from minimal I think.

When this is merged I'll revisit #28935 too.

jgraham · 2021-05-25T12:30:51Z

tools/wptrunner/wptrunner/config.py


 here = os.path.dirname(__file__)

-class ConfigDict(dict):
+class ConfigDict(Dict[str, Any]):


This feels weird, but I guess it's right.

Do you mean it's weird to use a type as the base class? I agree, things don't work quite as I first guessed with inheritance. It's not possible to do dict: Dict[str, Any], that's a syntax error. My guess is since that aside from ABCs and maybe generics (if Python has those) inheritance is from a specific type, not a type in the mypy sense.

And typing.Dict can be used like this, by some magic I haven't inspected closely.

jgraham · 2021-05-25T12:31:52Z

tools/wptrunner/wptrunner/executors/base.py

@@ -710,7 +720,7 @@ def after_connect(self):


 class WdspecProtocol(Protocol):
-    server_cls = None
+    server_cls = None  # type: ClassVar[Optional[Type[WebDriverServer]]]


Yeah, this is a design problem. #29089 will address that once it's ready.

jgraham · 2021-05-25T12:32:52Z

tools/wptrunner/wptrunner/executors/base.py

@@ -588,7 +598,7 @@ def get_screenshot_list(self, node, viewport_size, dpi, page_ranges):

 class WdspecExecutor(TestExecutor):
    convert_result = pytest_result_converter
-    protocol_cls = None
+    protocol_cls = None  # type: ClassVar[Type[Protocol]]


I've probably forgotten something but why aren't we using protocol_cls: ClassVar[Type[Protocol]] = None?

One would have to use Optional to allow that. For class variables, for some reason the rules are different when using the comment syntax. I can't find the docs for it now, but you can try it and see :)

foolip commented May 10, 2021

View reviewed changes

tools/wptrunner/wptrunner/executors/base.py Outdated Show resolved Hide resolved

moz-wptsync-bot added the mozilla:gecko-blocked label May 10, 2021

foolip commented May 10, 2021

View reviewed changes

tools/wptrunner/wptrunner/executors/base.py Outdated Show resolved Hide resolved

foolip commented May 10, 2021

View reviewed changes

tools/wptrunner/wptrunner/manifestupdate.py Outdated Show resolved Hide resolved

foolip commented May 10, 2021

View reviewed changes

tools/wptrunner/wptrunner/manifestupdate.py Outdated Show resolved Hide resolved

foolip mentioned this pull request May 10, 2021

Remove use of six.ensure_str/ensure_text in wptrunner #28935

Draft

Base automatically changed from foolip/mypy-tools to master May 11, 2021 08:30

foolip force-pushed the foolip/mypy-wptrunner branch 6 times, most recently from eac123b to e6994a6 Compare May 17, 2021 14:24

foolip commented May 17, 2021

View reviewed changes

tools/wptrunner/wptrunner/manifestupdate.py Outdated Show resolved Hide resolved

foolip marked this pull request as ready for review May 17, 2021 14:28

wpt-pr-bot added infra mypy.ini wptrunner The automated test runner, commonly called through ./wpt run labels May 17, 2021

wpt-pr-bot assigned foolip May 17, 2021

wpt-pr-bot requested a review from jgraham May 17, 2021 14:28

foolip force-pushed the foolip/mypy-wptrunner branch from e6994a6 to 044c143 Compare May 17, 2021 16:41

jgraham reviewed May 24, 2021

View reviewed changes

foolip force-pushed the foolip/mypy-wptrunner branch from 044c143 to 918f54c Compare May 24, 2021 13:33

foolip commented May 24, 2021

View reviewed changes

jgraham approved these changes May 25, 2021

View reviewed changes

foolip merged commit a5e1bc0 into master May 25, 2021

foolip deleted the foolip/mypy-wptrunner branch May 25, 2021 13:37

moz-wptsync-bot removed the mozilla:gecko-blocked label May 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run mypy for tools/wptrunner/ (but still allow untyped defs) #28942

Run mypy for tools/wptrunner/ (but still allow untyped defs) #28942

foolip commented May 10, 2021 •

edited

foolip May 10, 2021

foolip May 10, 2021

foolip commented May 17, 2021

foolip commented May 17, 2021

jgraham left a comment

foolip May 24, 2021

jgraham May 25, 2021

foolip commented May 24, 2021

jgraham May 25, 2021

foolip May 25, 2021

jgraham May 25, 2021

jgraham May 25, 2021

foolip May 25, 2021

		@@ -65,7 +65,6 @@ def env_options():


		class EdgeBrowser(Browser):
		used_ports = set()

	class GeckoDriverProtocol(WdspecProtocol):
	server_cls = None # To avoid circular imports we set this at runtime

Run mypy for tools/wptrunner/ (but still allow untyped defs) #28942

Run mypy for tools/wptrunner/ (but still allow untyped defs) #28942

Conversation

foolip commented May 10, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foolip commented May 17, 2021

foolip commented May 17, 2021

jgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foolip commented May 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foolip commented May 10, 2021 •

edited