Skip to content

Modify HTTPretty to allow for assertion testing#19

Merged
ajhodges merged 12 commits intomasterfrom
feature/httpretty-asserter-mixin
Mar 20, 2020
Merged

Modify HTTPretty to allow for assertion testing#19
ajhodges merged 12 commits intomasterfrom
feature/httpretty-asserter-mixin

Conversation

@jamesfneyer
Copy link
Copy Markdown
Contributor

@jamesfneyer jamesfneyer commented Feb 3, 2020

When running pytests, there is not a very clear and easy way to test that all your mocked calls have been made, and if they have been made with the expected data. This provides a way to ensure that all the calls were made as expected, via passing in a list of assertions to HTTPretty.
This is run by using the HTTPrettyAsserter in a yield fixture, and passing in a list of assertions to the assert_calls function on the mocked calls.

Copy link
Copy Markdown
Contributor

@ajhodges ajhodges left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of this feature, but I'm not sure it belongs in a mixin. Seems to me like a decorator would be more appropriate. I think it'd be cleaner and a little more flexible. Something like:

@httpretty_assertions([{'path': '/bing-path', 'query': {'param_1': 1}}])

Where the position drives the asserted order, not an index parameter. Or alternatively you could write a HTTPretty class that inherits from the base one and provides a httpretty.assert_calls() method.

@mlclay also had the idea to add something similar to the functionality in https://github.com/kavod/WWWoman where we can define the mocked urls and assertions together in an annotation.

This is obviously a big change and will need to be factored into future sprint planning.

Comment thread tests/test_httpretty_asserter.py Outdated
@jamesfneyer jamesfneyer changed the title Add HTTPretty Asserter mixin for Pytest Modify HTTPretty to allow for assertion testing Feb 6, 2020
@jamesfneyer jamesfneyer requested a review from ajhodges February 6, 2020 16:28
@jamesfneyer jamesfneyer force-pushed the feature/httpretty-asserter-mixin branch from cdcda70 to 11b9a4d Compare February 10, 2020 19:58
@ajhodges
Copy link
Copy Markdown
Contributor

Were you happy with this? I remember you said there were some changes you wanted to make to mine.

@jamesfneyer
Copy link
Copy Markdown
Contributor Author

Thinking about it, I'm curious as to whether you believe that the path should always be required in the assertion? does it make sense to check that when there is no path?

@ajhodges
Copy link
Copy Markdown
Contributor

ajhodges commented Mar 4, 2020

What do you mean by "does it make sense to check that when there is no path"? I don't know how you could make HTTPretty assertions without a URL.

Also, it looks like the commits on this branch got screwed up somehow. I am seeing changes in the diff that are already in master. Please rebase.

@jamesfneyer
Copy link
Copy Markdown
Contributor Author

Basically, the path is checking everything after the .com or whatever ending, so for things like calling on the url shortener we are just hitting t.shipchain.io with no path at the end and I was wondering if it made sense to still require a path check when it would be blank

@ajhodges
Copy link
Copy Markdown
Contributor

ajhodges commented Mar 4, 2020

If a request is made to t.shipchain.io, the path is /

@jamesfneyer
Copy link
Copy Markdown
Contributor Author

And in those instances does it still make sense to require that as a parameter in the assertion? It couldn't hurt but I was just thinking that it might seem like an unnecessary check

@ajhodges
Copy link
Copy Markdown
Contributor

ajhodges commented Mar 4, 2020

Yes, assertions should be explicit

@jamesfneyer jamesfneyer force-pushed the feature/httpretty-asserter-mixin branch 2 times, most recently from d598e98 to fcd2ee1 Compare March 5, 2020 18:33
Copy link
Copy Markdown
Contributor

@ajhodges ajhodges left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mlclay made a good point - I think this would be nice to have as part of our pytest plugin (in the same way that json_asserter was). This would make usage a little easier. Maybe name it httpretty_asserter. I would not make it an autouse fixture though.

Copy link
Copy Markdown
Contributor

@ajhodges ajhodges left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please squash some of these commits from the refactor that was rolled back? It's ready for merge after that.

@jamesfneyer jamesfneyer force-pushed the feature/httpretty-asserter-mixin branch from e7386af to 75f30ee Compare March 20, 2020 14:21
@jamesfneyer jamesfneyer force-pushed the feature/httpretty-asserter-mixin branch from 75f30ee to 3702247 Compare March 20, 2020 14:42
@ajhodges ajhodges merged commit a6d87dc into master Mar 20, 2020
@ajhodges ajhodges deleted the feature/httpretty-asserter-mixin branch March 20, 2020 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants