4115 - Fix CI Typechecks

[hacklschorsch]

Fix Type checks on CircleCi.

Fixes ticket: https://tahoe-lafs.org/trac/tahoe-lafs/ticket/4115

[coveralls]

coverage: 95.443% (+0.06%) from 95.383%
when pulling 7417cdb on hacklschorsch:4115.fix-ci-typechecks
into 6cf6747 on tahoe-lafs:master.

[meejah]

Sure, this makes it green .. but it changes the semantics?

That is, _treq can really be any of "the module" or StubTreq or HTTPClient -- but the types defined in "treq" are incorrect to actually allow this notion to be captured.

See also twisted/treq#243 and twisted/treq#244

Put another way: if we want to change the type to demand HTTPClient then we need all usages of it to also be re-written to use HTTPClient (only) and not treq.* or StubTreq (which also seems to be what "treq" itself is at least "encouraging" at the moment).

[hacklschorsch]

Put another way: if we want to change the type to demand HTTPClient then we need all usages of it to also be re-written to use HTTPClient (only) and not treq.* or StubTreq (which also seems to be what "treq" itself is at least "encouraging" at the moment).

I don't undertstand. Shouldn't mypy now complain about when we set StorageClient._treq to anything that isn't an HTTPClient? Is that not what mypy is about?

See also twisted/treq#243 and twisted/treq#244

These are from 2019. StubTreq (or StubClient) we use once, in a test; And issue 244 - "start encouraging people to use HTTPClient and not module-global stuff" - I interpret as we should have been doing all along what this PR does.

Where am I wrong?

[meejah]

These are from 2019. StubTreq (or StubClient) we use once, in a test; And issue 244 - "start encouraging people to use HTTPClient and not module-global stuff" - I interpret as we should have been doing all along what this PR does.

We are using StubTreq and this PR does not fix that.

[hacklschorsch]

We are using StubTreq and this PR does not fix that.

But only once, in a test, and there MyPy complained about something different - that StubTreq does not provide .request() - So seemingly there's a problem with how StubTreq is a Stub for Treq? If it doesn't provide the same interface? I mean? If I was a type checker, I would complain about that as well

[hacklschorsch]

We are using StubTreq and this PR does not fix that.

This PR fixes https://tahoe-lafs.org/trac/tahoe-lafs/ticket/4115, which is about type checks.

[meejah]

Well, it does make it green, by ignoring the type-check on StubTreq (right?)

[hacklschorsch]

Well, it does make it green, by ignoring the type-check on StubTreq (right?)

That is one of three type problems this PR fixes, yes.

It's the least important and interesting one of the three, IMHO: It's a one-time use in our test code. Should that ever fail at runtime it's just a failed unit test to fix.

The type-check this ignores is that MyPy (erroneously) thinks StubTreq is not providing .request() (attr-defined), which is because MyPy has a wrong idea of StubTreq. This PR doesn't fix Treq, Twisted or MyPy; I just fixes our typecheck CI suite.

[hacklschorsch]

Should I remove the ignore in the test code so you can fix that in a separate PR?

[meejah]

The type-check this ignores is that MyPy (erroneously) thinks StubTreq is not providing .request() (attr-defined), which is because MyPy has a wrong idea of StubTreq. This PR doesn't fix Treq, Twisted or MyPy; I just fixes our typecheck CI suite.

Right, okay. Those tickets above say some things about this of course (that, yes, StubTreq doesn't type-check "properly" -- I think because it's trying to pretend to be both the module, and HTTPClient).

What I'm getting at is that the correct fix is to only use HTTPClient in tahoe-lafs (which would be forward-compatible with where the "treq" project appears to be heading).

[hacklschorsch]

I am confused. Is that not what this change is ensuring? MyPy should from now on complain whenever we don't use HTTPClient here.

[hacklschorsch]

What I'm getting at is that the correct fix is to only use HTTPClient in tahoe-lafs (which would be forward-compatible with where the "treq" project appears to be heading).

Is this your required bar to merge this PR or can this wait until Treq deprecates the treq() API?

[meejah]

I am confused. Is that not what this change is ensuring? MyPy should from now on complain whenever we don't use HTTPClient here.

Approximately yes, but then where it did complain you just ignored it.

If that's really the only use of StubTreq can it be eliminated?

can this wait until Treq deprecates the treq() API?

Not sure what you mean here.

[hacklschorsch]

But that was about MyPy not knowing StubTreq well enough to know we can call a method on it, not about our API having not the right type annotations.

[hacklschorsch]

If I understand correctly, we should be able to use HTTPClient instead of StubTreq according to the treq docs, but we can't, since while this pacifies MyPy it breaks the unit test at runtime.

I tried for a while but couldn't find a way to use HTTPClient here with the tests working.

I revert to my earlier fix and again argue that MyPy is at fault here, not our code, and that my making it ignoring its wrong assumption that StubTreq has no .request() method is an acceptable fix.

If you don't like it @meejah please find a better fix, I am done wasting my time with this.

[meejah]

Yes, that's what I was getting at: can we just replace StubTreq there with HTTPClient?

Sounds like you're saying you tried now, and "no, we can't replace it"?

[meejah]

Probably filing a followup ticket so we don't "forget" to migrate to HTTPClient would be good.

Also, this failure on the "another-locale" builder looks probably-real and related to this?

testtools.testresult.real._StringException: testtools.testresult.real._StringException: Traceback (most recent call last):
  File "/tmp/tahoe-lafs.tox/py39/lib/python3.9/site-packages/twisted/internet/defer.py", line 209, in maybeDeferred
    result = f(*args, **kwargs)
  File "/tmp/tahoe-lafs.tox/py39/lib/python3.9/site-packages/testtools/testcase.py", line 704, in _run_test_method
    return self._get_test_method()()
  File "/tmp/tahoe-lafs.tox/py39/lib/python3.9/site-packages/allmydata/test/eliotutil.py", line 156, in run_with_logging
    return test_method(*args, **kwargs)
  File "/tmp/tahoe-lafs.tox/py39/lib/python3.9/site-packages/allmydata/test/test_storage_http.py", line 720, in test_missing_authentication
    client.request(
  File "/tmp/tahoe-lafs.tox/py39/lib/python3.9/site-packages/treq/client.py", line 267, in request
    d = wrapped_agent.request(
  File "/tmp/tahoe-lafs.tox/py39/lib/python3.9/site-packages/twisted/web/client.py", line 1480, in request
    deferred = self._agent.request(method, uri, headers, bodyProducer)
  File "/tmp/tahoe-lafs.tox/py39/lib/python3.9/site-packages/twisted/web/client.py", line 1573, in request
    deferred = self._agent.request(method, uri, headers, bodyProducer)
  File "/tmp/tahoe-lafs.tox/py39/lib/python3.9/site-packages/twisted/web/client.py", line 1352, in request
    d = self._agent.request(method, uri, headers, bodyProducer)
AttributeError: 'KleinResource' object has no attribute 'request'

[meejah]

I made a PR-to-this-PR showing how to fix at least that one instance (longer-term we should move as much as possible to this paradigm, which is what treq developers prefer as per the comments on their issue tracker).

hacklschorsch#1

[hacklschorsch]

Sounds like you're saying you tried now, and "no, we can't replace it"?

That was my finding, but seemingly you proved me wrong in hacklschorsch#1. Is your patch equivalent to what StubTreq does or is it different? I don't know enough about Twisted, Twisted.Trial or Treq to understand what's going on. Seemingly the right reactor is chosen when run in Twisted.Trial? And StubTreq has gained some functionality since it was introduced, but nothing we use here?

[hacklschorsch]

Also, this failure on the "another-locale" builder looks probably-real and related to this?

That is new and happened with HTTPClient instead of StubTreq - but I wasn't supplying RequestTraversalAgent like you did, so that seems to be one of the requirements to use HTTPClient instead of StubTreq.