chore: Stop running mypy on tests #5586
Conversation
Often times we need to test error conditions or invalid input. That necessitates doing things that mypy considers unsound. While mypy can also provide some benefit to testing, the cost of silencing errors isn't worth the benefit.
|
after having to do weird things for the ibm datasources tests, +1 |
There was a problem hiding this comment.
It does sometimes feel a bit like busy-work to be updating typing on unittests tests.
I see a bit more value in still running mypy on the integration tests which can be exposed to using pycloudlib and other library dependencies where we may more likely misuse some dependencies.
What do you think about the middle ground of dropping mypy from tests/unittests but still including tests/integration_tests subdir?
There was a problem hiding this comment.
I commiserate with the complaints about mypy and Python typing that others have made.
While mypy can also provide some benefit to testing, the cost of silencing errors isn't worth the benefit. (@TheRealFalcon)
after having to do weird things for the ibm datasources tests, +1 (@a-dubs)
It does sometimes feel a bit like busy-work to be updating typing on unittests tests. (@blackboxsw)
It can feel tedious and slow, and certainly provides more benefit to runtime code than to tests. That said, I'd like to highlight the value that I see from tests.
When when working out Python types on pre-existing tests, I sometimes find that we are testing code paths which cannot run at runtime. These discoveries seem to fall into two categories:
- the original authors were testing additional code paths out of fear that certain types were possible that mypy can now tell us isn't going to happen
- the test writers incorrectly assumed the types of the runtime code
Mypy assists us by increasing our confidence that the code that we are testing is receiving inputs that they will receive at runtime - so I think that dropping mypy will actually decrease the quality of our tests over time. Due to the nature of dynamic typing, even if we had 100% coverage, the actual coverage of types that cloud-init sees at runtime could be much lower than 100%.
|
@holmanb , you make some good points, but contrast that with this sort of thing: def parse_json(my_json) -> Optional[Dict]:
...
def read_file(filename, decode) -> Union[str, bytes]:
...
def test_json():
x = parse_json('{"a": 1}')
assert x['a'] == 1
def test_file():
x = read_file('file.txt', True)
assert x == 'content'We either need to sprinkle def test_json():
x = parse_json('{"a": 1}')
if x is None:
pytest.fail("oh noes")
assert x['a'] == 1I suspect we'll see more of the latter as we'll want to "properly" silence errors rather than just ignoring them. To me this is worse than silencing errors because we're needlessly bloating tests testing for conditions that are implicitly part of the assertion that already exists. Given how much of the cloud-init code base returns Optionals and Unions, I think this will be too common of a problem. |
For optionals it usually suffices to toss in an def parse_json(my_json) -> Optional[Dict]:
...
def test_json1():
x = parse_json('{"a": 1}')
assert x and x['a'] == 1
def test_json2():
x = parse_json('{"a": 1}') or {}
assert x['a'] == 1Neither of these feels particularly burdensome to me. The unions are more annoying because of mypy's strict union checking. Some projects deal with this by avoiding Union types altogether. The workaround that typeshed uses is to use I try to avoid |
|
@aciba90 You've done a fair bit of typing work in cloud-init. I'd be curious to know your thoughts on the topic. |
|
@holmanb when he's right ⣀⣀⣀⣀⣀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠄⠀⠀⢀⠀⠀⠀⠀⠀⠐⠀⢢⠆⠐⠁⠀⠀⠀⠀⠇⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀ |
|
Good points @holmanb . I'll close this. |
This should be ready to review but in draft state as it may be controversial.
Proposed Commit Message
Additional Context
Test Steps
Merge type