Utility for providing a sequence of requests/responses to StubTreq #106

cyli · 2015-07-13T23:49:43Z

This was originally in #96, and removed to make review easier. This undoes that remove.

Part of addressing #27.

Again, planning on writing docs in another PR, as this PR is fairly large already and may get slightly larger depending on review comments.

…n be reverted in a future PR to add this functionality back in." This reverts commit 75b49b1.

cyli · 2015-07-13T23:52:37Z

Couple questions for the reviewer:

StubTreq(StringStubbingResource(SequenceStringStubs([...]) is pretty verbose - I wanted to make a nicer wrapper like sequence_stub([...]) - thoughts on naming/whether this makes it so we litter the testing module with every type of stubber.
I'm not super happy about storing the failures and having to test that, but I don't think there's any other way, since Resource.render will eat any other exception and just return a 500, and getting around that is pretty hard. Any suggestions?
@hynek suggested putting all the interfaces in a _interfaces.py file and just ignoring that entire file rather than sprinkling the codebase with # pragma: no cover comments, which seems like a good idea. But it might be useful to expose this interface from the testing module too? In case someone wanted to make their own StringStubber. Should we do that?
Not so sure of using a sentinel value of None in SequenceStringStubs to mean "anything". Wondering if we should include an ANY object, like mock.ANY?

codecov-io · 2015-07-13T23:53:28Z

Current coverage is `96.16%`

Merging #106 into master will increase coverage by +0.34% as of 6d11011

Coverage Diff

@@            master    #106   diff @@
======================================
  Files           19      19       
  Stmts         1411    1535   +124
  Branches       112     121     +9
  Methods          0       0       
======================================
+ Hit           1352    1476   +124
  Partial         19      19       
  Missed          40      40

Powered by Codecov

codecov-io · 2015-07-13T23:54:41Z

Current coverage is `96.16%`

Merging #106 into master will increase coverage by +0.34% as of 6d11011

Coverage Diff

@@            master    #106   diff @@
======================================
  Files           19      19       
  Stmts         1411    1535   +124
  Branches       112     121     +9
  Methods          0       0       
======================================
+ Hit           1352    1476   +124
  Partial         19      19       
  Missed          40      40

Powered by Codecov

codecov-io · 2015-07-13T23:55:33Z

Current coverage is `96.18%`

Merging #106 into master will increase coverage by +0.35% as of 71267b4

@@            master    #106   diff @@
======================================
  Files           19      19       
  Stmts         1417    1548   +131
  Branches       113     121     +8
  Methods          0       0       
======================================
+ Hit           1358    1489   +131
  Partial         19      19       
  Missed          40      40

Review entire Coverage Diff as of 71267b4

Powered by Codecov. Updated on successful CI builds.

cyli · 2015-07-14T01:17:12Z

That's odd - why did codecov comment 3 times on the same commit?

glyph · 2015-07-14T04:56:43Z

I like Hynek's suggestion, except for the fact that it gives the interfaces the wrong module name; they are relocated in the docs, but they repr as having an underscore in them which is misleading.

So you might also want to set __name__ before the interfaces are defined:

>>> from zope.interface import Interface
>>> 
>>> class ISomething(Interface):
...     pass
... 
>>> ISomething
<InterfaceClass __main__.ISomething>
>>> __name__ = 'hello'
>>> class ISomething(Interface):
...     pass
... 
>>> ISomething
<InterfaceClass hello.ISomething>

glyph · 2015-07-14T05:23:44Z

StubTreq(StringStubbingResource(SequenceStringStubs([...]) is pretty verbose - I wanted to make a nicer wrapper like sequence_stub([...]) - thoughts on naming/whether this makes it so we litter the testing module with every type of stubber.

Clearly we should have a higher-level utility function. But we can add those later; I think that trying to block this on committing to something higher level would really slow down progress.

glyph · 2015-07-14T05:33:52Z

I'm not super happy about storing the failures and having to test that, but I don't think there's any other way, since Resource.render will eat any other exception and just return a 500, and getting around that is pretty hard. Any suggestions?

I think that burdening the caller with performing this check themselves every time is indeed the wrong thing to do: anywhere you say "and then the caller must..." is an opportunity for a mistake to be made.

What do you think of this, instead: require a TestCase as an argument to StringStubbingResource, and then instead of doing self.test_case.fail(...), do self.test_case.addCleanup(lambda: self.test_case.fail)? This puts the test failure in a context that reports directly to TestCase and doesn't get caught by intermediary resource-rendering code.

You can pass the TestCase along either to StringStubbingResource or to SequenceStringStubs. In either case you should be able to eliminate the failures list; if you pass it to StringStubbingResource you can get rid of it entirely, if you pass it to SequenceStringStubs, you can replace it with a simpler addFailure reporting protocol; no need to couple to all the methods of list in the interface.

glyph · 2015-07-14T05:34:40Z

Not so sure of using a sentinel value of None in SequenceStringStubs to mean "anything". Wondering if we should include an ANY object, like mock.ANY?

Explicit is better than implicit, so, "yes".

@hynek

Thanks @hynek

glyph · 2015-07-14T06:50:33Z

Sorry, to be clear, when I said "addFailure reporting protocol", I should have said ISequenceStringStubs.add_failure or maybe add_request_failure; StringStubbingResource would report the failure to ISequenceStringStubs, and then it would report it to the TestCase via addCleanup. I didn't mean addFailure from IReporter.

@glyph

…n the cleanup Thanks @glyph

… codecov, so add it to coverage run too.

cyli · 2015-07-17T01:14:16Z

So is this a bogus report? https://codecov.io/github/twisted/treq/treq/test/test_testing.py?ref=d5b7c708f9e7794b6964f8202a9b6cde39ad67e3#l-235

The tests pass, so it should have gone through everything in that loop - what coverage am I missing?

glyph · 2015-07-17T01:23:17Z

Hey @hynek - crappy as coveralls has been, it at least has a coverage delta browser. Figuring out why codecov is unhappy with this code is really obscure, since it increases coverage. I'm inclined to put treq back on coveralls based on the experience in this PR. Is there some part of the UI that I'm missing?

cyli · 2015-07-17T01:30:53Z

Apparently you can't use for loops in tests if we want branch coverage on the tests also? Is there any way we can get branch coverage on the source but line coverage on the tests themselves?

glyph · 2015-07-17T01:46:36Z

I filed https://bitbucket.org/ned/coveragepy/issues/383/ to hopefully address one of the issues here.

hynek · 2015-07-17T05:14:36Z

Treq never was on coveralls.

If you have particular problems it might be worth to bring it upstream. @dstufft or @reaperhulk told me the devs are receptive (contrary to coveralls which appears to be abandonware)

radix · 2015-07-17T19:56:13Z

treq/interfaces.py

+class IStringResponseStubs(Interface):
+    """
+    An interface that :class:`StringStubbingResource` expects to provide it
+    with a response based on what the


docstring cut off here

radix · 2015-07-17T19:57:12Z

Personally I think the whole interface is overkill and it's unnecessary. It only has one method, so you can just instead take a function as an argument.

cyli · 2015-07-17T20:03:38Z

@radix: Good point, thanks - it went through several iterations where it had maybe failure reporting, but you're right, as it stands there is no reason to have an interface.

…elp with codecov, so add it to coverage run too." This reverts commit 3bd122c.

glyph · 2015-07-17T21:03:51Z

Python doesn't really have a way to describe the signature of a function that's expected except for Interface-with-a-__call__ method, though.

cyli · 2015-07-17T21:09:36Z

@glyph Can we just describe it in the StringStubbingResource docstring? It's not like we do enforcement of the interface anyway.

glyph · 2015-07-17T21:23:32Z

@cyli - Sure, if you like it better, I'm not going to be picky about it. The main thing that bothers me is that neither epytext nor ReST has a way to nest :param: descriptions, but I admit that the interface is just a non-ideal tradeoff in a different direction: we really are just describing the parameter to just this one function, not a general interface that can be passed all kinds of places, so making the user jump out and read a different class's docstring is unpleasant in a different way than having ad-hoc parameter descriptions instead of formal markup.

glyph · 2015-07-17T21:28:14Z

I hear @radix likes haskell, maybe he will port haskell to python so we can use the eminently readable notation string_response_stubs :: HTTPMethod -> URL -> URLParameters -> HTTPHeaders -> Bytes -> IResponse instead

@radix

…_for to __call__ - thanks @radix

…have it take failure reporters for now.

cyli · 2015-08-03T21:11:08Z

Note to reviewer: I plan on adding a utility like:

@contextmanager
def stub_request_sequence(sequence, failure_reporter=None, assert_consumed=True):
    """
    Convenience context manager that returns a stubbed treq.  Once the context
    exits, any async stubbing errors are reported.  By default, also fails
    if not all requests were consumed.

    Usage::
        with stub_request_sequence([]) as stubtreq:
            stubtreq.get(...)
    """

in the next PR, and the default failure reporter is to raise an AssertionError. I'm happy to just put it into this PR, but the PR seems pretty large already, and this seems like the next level up abstraction layer.

glyph · 2015-08-04T04:31:08Z

@cyli - is the recent codecov result bogus?

cyli · 2015-08-04T04:40:31Z

@glyph I think so? I've only edited two files in this PR: testing.py and test_testing.py, and both are at 100%. Here is the patch coverage (I think): https://codecov.io/github/twisted/treq/commit/c1ce2756aa6bf8e0d70d8286ae79101e43e317e4

glyph · 2015-08-04T04:43:30Z

https://codecov.io/github/twisted/treq says 95.84%, so this result doesn't make sense. Why is it comparing to 96.25%?

glyph · 2015-08-04T04:52:18Z

Hey @codecov-io - can you help us out with this?

stevepeak · 2015-08-04T14:02:17Z

Sorry we don't get pings from @codecov-io

Can you make an empty commit (git commit --allow-empty -m "rebuild codecov report") on this PR, I'd like to download the raw upload and review its content. Thank you!

cyli · 2015-08-04T18:29:38Z

@stevepeak Done

stevepeak · 2015-08-04T20:24:32Z

@cyli thank you! The system was taking the project coverage from an incorrect reference point. I've fixed this issue. Posted a successful status for this build to reflect that.

Thank you and I apologize for the issue.

reaperhulk · 2015-08-04T20:25:29Z

@stevepeak was this a systemic issue you've corrected? We've been seeing this on pyca/cryptography as well :)

cyli · 2015-08-04T20:34:38Z

Thanks @stevepeak for the quick response and fix!

stevepeak · 2015-08-04T20:40:25Z

@reaperhulk for a window of about 2 days I was tying a new technique for the Project status and fetching the previous commit's coverage because there was a root issue: the first commit on a new branch could not find its "previous" coverage. My attempt to fix this caused this issue.

Furthermore I'm working on improving the Project status to have an option of not setting the "minimum" coverage, instead the status would simply be: Did it lower coverage at all? A success would be the coverage of this commit did not lower coverage.

glyph · 2015-08-21T23:04:27Z

treq/testing.py

+
+    def __init__(self, get_response_for):
+        """
+        :param get_response_for: A callable that takes 5 parameters:


Rather than re-describing this parameter, I'd just say "See ``StringStubbingResource``.", since the parameter is already described in detail there.

glyph · 2015-08-21T23:08:29Z

@cyli - modulo the documentation nitpicking, I am fairly happy with this. Address and land, please?

glyph · 2015-08-27T22:39:22Z

treq/testing.py

+
+            sequence_stubs = RequestSequence([...])
+            stub_treq = StubTreq(StringStubbingResource(sequence_stubs))
+            with sequence_stubs.consume():


Oops! This example is wrong. sequence_stubs.consume() requires a sync_failure_reporter and this doesn't pass one.

cyli · 2015-08-31T09:08:31Z

This PR is cursed. :| Some brokenness with travis and cryptography.

mithrandi · 2015-08-31T11:35:23Z

Cryptography builds an empty wheel on pypy < 2.6 due to a too-old version of cffi; this wheel then gets cached on Travis making all other builds break. The next cryptography release will error out instead, thus only breaking the build on PyPy (Travis has a too-old version) rather than everywhere. Something will still need to be done about the PyPy builds, though.

glyph · 2015-08-31T21:44:57Z

This PR is cursed. :| Some brokenness with travis and cryptography.

It's not just this PR. I think that all treq builds will start failing now; we need to figure this out just to un-block development.

Frankly I think the solution might be to de-support PyPy for now, pending Travis adding a more recent version, or at least someone submitting a travis config that acquires a more recent one.

Utility for providing a sequence of requests/responses to StubTreq

Revert "Remove some code, to make it easier to review. This commit ca…

29d8ffe

…n be reverted in a future PR to add this functionality back in." This reverts commit 75b49b1.

Move interface to a new file that is ignored by coverage

2b40602

Thanks @hynek

cyli added 5 commits July 14, 2015 00:31

Have SequenceStringStubs take a test case so it can report failures i…

3fba8d2

…n the cleanup Thanks @glyph

Add an ANY object to be used for matching against requests

8115b1c

Actually test the repr

f32db5d

Remove dead code.

8e38158

Ok, omitting interfaces.py from the coverage report doesn't help with…

3bd122c

… codecov, so add it to coverage run too.

radix reviewed Jul 17, 2015
View reviewed changes

Revert "Ok, omitting interfaces.py from the coverage report doesn't h…

bdac492

…elp with codecov, so add it to coverage run too." This reverts commit 3bd122c.

cyli added 2 commits August 3, 2015 13:34

Rename StringSequenceStubs to RequestSequence and change get_response…

16c1882

…_for to __call__ - thanks @radix

Remove RequestSequence's dependency on unit test TestCases, and just …

4867d98

…have it take failure reporters for now.

rebuild codecov report

a2c8d67

glyph reviewed Aug 21, 2015
View reviewed changes

glyph added the good to land (with minor changes) label Aug 21, 2015

glyph assigned cyli Aug 21, 2015

glyph reviewed Aug 27, 2015
View reviewed changes

Fix docstrings as per @glyph's review comments.

1bb98f3

glyph mentioned this pull request Aug 31, 2015

No pypy for now #113

Closed

Merge branch 'master' of github.com:twisted/treq into stringstubs-treq

ad511e2

cyli added a commit that referenced this pull request Sep 6, 2015

Merge pull request #106 from cyli/stringstubs-treq

82ec603

Utility for providing a sequence of requests/responses to StubTreq

cyli merged commit 82ec603 into twisted:master Sep 6, 2015

cyli deleted the stringstubs-treq branch September 7, 2015 04:09

Utility for providing a sequence of requests/responses to StubTreq #106

Utility for providing a sequence of requests/responses to StubTreq #106

Conversation

cyli commented Jul 13, 2015

cyli commented Jul 13, 2015

codecov-io commented Jul 13, 2015

Current coverage is 96.16%

Coverage Diff

codecov-io commented Jul 13, 2015

Current coverage is 96.16%

Coverage Diff

codecov-io commented Jul 13, 2015

Current coverage is 96.18%

cyli commented Jul 14, 2015

glyph commented Jul 14, 2015

glyph commented Jul 14, 2015

glyph commented Jul 14, 2015

glyph commented Jul 14, 2015

glyph commented Jul 14, 2015

cyli commented Jul 17, 2015

glyph commented Jul 17, 2015

cyli commented Jul 17, 2015

glyph commented Jul 17, 2015

hynek commented Jul 17, 2015

radix Jul 17, 2015

Choose a reason for hiding this comment

radix commented Jul 17, 2015

cyli commented Jul 17, 2015

glyph commented Jul 17, 2015

cyli commented Jul 17, 2015

glyph commented Jul 17, 2015

glyph commented Jul 17, 2015

cyli commented Aug 3, 2015

glyph commented Aug 4, 2015

cyli commented Aug 4, 2015

glyph commented Aug 4, 2015

glyph commented Aug 4, 2015

stevepeak commented Aug 4, 2015

cyli commented Aug 4, 2015

stevepeak commented Aug 4, 2015

reaperhulk commented Aug 4, 2015

cyli commented Aug 4, 2015

stevepeak commented Aug 4, 2015

glyph Aug 21, 2015

Choose a reason for hiding this comment

glyph commented Aug 21, 2015

glyph Aug 27, 2015

Choose a reason for hiding this comment

cyli commented Aug 31, 2015

mithrandi commented Aug 31, 2015

glyph commented Aug 31, 2015

Current coverage is `96.16%`

Current coverage is `96.16%`

Current coverage is `96.18%`