[py] make mypy only complain about stuff that matters

[colons]

Description

This PR disables the mypy checks that don't matter for a codebase that is only beginning to adopt type checks, in the hopes that flagrantly incorrect annotations such as the ones fixed in #10120 and #10172 can be avoided. It also fixes a number of other very wrong annotations, although not all of them. Some would be hard to fix without removing them entirely, refactoring, or breaking backwards compatibility, and I did not want to do that here without raising it first. and adds a now-passing mypy check to CI.

By has been turned into an Enum, although methods which accepted string representations of locator strategies still do, and the tests have been expanded to guarantee that. Keys is probably worth treating the same, but I have not done that here.

I have added more annotations in places where doing so helped with testing. I will probably have more PRs in the near future, in particular to annotate the tests.

The only remaining CI failures appear to be also happening in trunk.

Also, one of the things mypy is complaining about is that WheelInput is given too many arguments in action_builder.ActionBuilder.add_wheel_input, which I'm confused about. Is there some magic I'm missing, or is this just a method that isn't working at the moment? raised in #10210

Motivation and Context

I observed in a comment that the errors that have been made in the existing annotations are completely understandable given how overwhelmingly noisy the current mypy checks are. In an existing codebase, complaining about stuff like missing return annotations is pointless; all you really want to do is add annotations as you work on stuff and make sure that what you do add is correct.

Incorrect annotations are way, way worse than no annotations.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[codecov-commenter]

Codecov Report

Merging #10202 (840c5b3) into trunk (878feb7) will decrease coverage by 25.88%.
The diff coverage is 66.80%.

@@             Coverage Diff             @@
##            trunk   #10202       +/-   ##
===========================================
- Coverage   52.24%   26.35%   -25.89%     
===========================================
  Files          84      181       +97     
  Lines        5482    12896     +7414     
  Branches      272      209       -63     
===========================================
+ Hits         2864     3399      +535     
- Misses       2346     9288     +6942     
+ Partials      272      209       -63     

Impacted Files	Coverage Δ
py/generate.py	14.55% <0.00%> (ø)
py/selenium/webdriver/common/bidi/cdp.py	19.26% <0.00%> (-5.62%)	⬇️
py/selenium/webdriver/common/utils.py	98.21% <0.00%> (+1.60%)	⬆️
py/selenium/webdriver/remote/mobile.py	15.15% <0.00%> (ø)
py/selenium/webdriver/safari/service.py	50.00% <0.00%> (-4.17%)	⬇️
...ium/webdriver/common/executing_javascript_tests.py	1.54% <ø> (ø)
...lenium/webdriver/remote/remote_connection_tests.py	3.31% <ø> (ø)
...thenticator/virtual_authenticator_options_tests.py	2.04% <0.00%> (ø)
py/selenium/webdriver/chromium/webdriver.py	51.76% <16.66%> (+2.35%)	⬆️
py/selenium/webdriver/firefox/options.py	52.94% <33.33%> (+2.94%)	⬆️
... and 157 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[titusfortner]

This is a lot of work, thank you!

There are a couple browser tests that seem to be flaky in Python suite, but in general CI should be passing, especially the unit tests. Can you fix: https://github.com/SeleniumHQ/selenium/runs/4668655570?check_suite_focus=true#step:7:187

I'm pretty new to typing and mypi, can we surface the issues from PRs in a github action? What do we need to add? https://github.com/SeleniumHQ/selenium/blob/trunk/.github/workflows/python.yml

When you remove all of those checks in mypi.ini what is left, just actual errors? Do we want to create an issue to put those back in after we get things migrated?

[colons]

There are a couple browser tests that seem to be flaky in Python suite, but in general CI should be passing, especially the unit tests. Can you fix: https://github.com/SeleniumHQ/selenium/runs/4668655570?check_suite_focus=true#step:7:187

oh heck, python 3.7 doesn't have typing.TypedDict! fixed in 049ced29e163b1d8be9c3cd831573a74afb4d4d0, i think, using the same method TypedDict imports are wrapped elsewhere in the codebase

I'm pretty new to typing and mypi, can we surface the issues from PRs in a github action? What do we need to add? https://github.com/SeleniumHQ/selenium/blob/trunk/.github/workflows/python.yml

mypy already runs in CI; to make the job fail when annotations are wrong, you'd need to get rid of the || true from the mypy job there. i didn't do that here because, as i say in the description, there are some issues mypy is still complaining about that will require the kind of work a project maintainer should be involved with to fix

When you remove all of those checks in mypi.ini what is left, just actual errors? Do we want to create an issue to put those back in after we get things migrated?

what's left is errors in things that have explicit annotations; unannotated functions are ignored

i'd defer to the mypy docs' recommendations about existing codebases, but that's probably something to play by ear over time; as your typing coverage increases, maybe try turning stuff on occasionally and see how much there is to fix

[titusfortner]

This needs to be reverted. The characters need to be sent one at a time, not as a string, it's causing the 3 interactions errors that include send_keys

[colons]

i'm confused about how this works, though; Keys is just a class with a bunch of attributes, not an enum or anything, so i'm not sure how this isinstance could ever be truthy before this change

>>> from selenium.webdriver.common.keys import Keys
>>> isinstance(Keys.LEFT, Keys)
False

if this function needs to be sure that keys are sent one character at a time, it seems to me like it should explicitly check the length of the string, not whether it's an instance of Keys

[colons]

i think removing this clause entirely is maybe the right move here

[titusfortner]

Yeah, I tried adding a few tests and seeing what it does, and I agree, short of passing in Keys() nothing will make it true. :)

[colons]

i think this is resolvable, too, if you agree with the change i made

[titusfortner]

This is causing test-remote execution to fail: https://github.com/SeleniumHQ/selenium/runs/4708398183

It looks like there's an UnboundLocalError issue because output isn't defined for condition: None in local_files

[colons]

i think i've fixed this in 41b47ea, but i'm a bit confused about this method in general; it seems like this function will perform dramatically different things based on whether one entry in a list of filenames is not a local file, and now that i know this machinery is here, i'm worried about accidentally matching file names from my current working directory and accidentally sending their contents to my test runners

i guess it probably doesn't matter that much, since it still sends the same data to the actual field, but it certainly feels like an unexpected interaction to happen, which scares me when that action is sending the contents of local files over a network. also, debugging a test failure due to this behaviour not happening when you expected it to seems like it'd potentially be pretty difficult

[titusfortner]

So, this has to do with file uploads. If the element is a file type input element and the string passed in send keys is a path to a file, the driver will handle it as an upload.

Look at Point 8 here for remote end steps for element send keys:
https://w3c.github.io/webdriver/#:~:text=the%20remote%20end%20steps%20for%20element%20send%20keys%20are

This gets weird when using a remote driver and the file is on a different system than the driver, which requires using a local file detector and uploading the file to the remote machine

[colons]

i figured as much, but if that's the case, i feel like this should check whether the field is a file input and then throw an error if one of the files doesn't exist, so that it's predictable and deterministic, instead of using this heuristic based on the local filesystem, and [removed because i didn't understand what "remote end" means]

...i still feel like this is a dangerous and surprising piece of behaviour, but i guess i'd have to raise this against the spec itself, and it doesn't have anything to do with this PR other than that i broke it

[titusfortner]

I haven't dug into the Python implementation. I'm wondering if it might make sense to have new methods in the bindings specifically for "upload" so we aren't overloading "send keys," but we're also looking at making a separate upload endpoint in w3c, so maybe that should be done first in case the user-facing implementation in the bindings is going to be different.

[colons]

i still don't think there's any action left to be taken here as part of this PR

[colons]

@symonk :)

(i don't think the remaining firefox failures are this branch's faulr)

[diemol]

@colons apologies for the lack of feedback. Wondering if you are still interested in this PR? If so, I can help to move it forward and review.

[isaulv]

Here some of my thoughts:
I would only use from typing_extensions import ... for all the type checking types. There's a good reason for this, and that is it will support newer constructs in later versions of Python in earlier versions of Python, and if there bug fixes it will be there instead of waiting for the next point release for the latest version of Python.
I wouldn't put too much effort in putting types in tests unless the helper functions are somewhat complex.

[diemol]

I am sorry about this. I am going to close this PR. It has not had activity in a long time, and the code base has moved, which is causing conflicts.

We are open to receiving more PRs about this, and hopefully, they are small, so they can get merged quickly.