Skip to content

Mock out requests in TestRequest unit tests#1794

Merged
freddyaboulton merged 3 commits into
mainfrom
fix-request-tests
Jul 18, 2022
Merged

Mock out requests in TestRequest unit tests#1794
freddyaboulton merged 3 commits into
mainfrom
fix-request-tests

Conversation

@freddyaboulton
Copy link
Copy Markdown
Collaborator

@freddyaboulton freddyaboulton commented Jul 14, 2022

Description

Requests unit tests were failing because https://reqres.in/api/users is unavailable. See this PR for an example: #1785

This PR mocks out the calls to that api to make the tests pass and not have to rely on external services

Checklist:

  • I have performed a self-review of my own code
  • My code follows the style guidelines of this project
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@freddyaboulton freddyaboulton marked this pull request as ready for review July 14, 2022 15:42
@freddyaboulton
Copy link
Copy Markdown
Collaborator Author

Lol looks like the service is back up again. I still think it's worthwhile to remove dependencies on external services from unit tests so I'll keep this PR open to hear what people think!

Comment thread test/test_utils.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's mock this request to "headers.jsontest.com" as well?

@abidlabs
Copy link
Copy Markdown
Member

Thanks for fixing this @freddyaboulton! Agreed that this is definitely the better approach. I noticed a couple of things:

@abidlabs
Copy link
Copy Markdown
Member

Thanks for fixing this @freddyaboulton! Agreed that this is definitely the better approach. I noticed a couple of things:

1 similar comment
@abidlabs
Copy link
Copy Markdown
Member

Thanks for fixing this @freddyaboulton! Agreed that this is definitely the better approach. I noticed a couple of things:

Comment thread test/test_utils.py
client_response: Request = await Request(
method=Request.Method.GET,
url="http://headers.jsontest.com/",
url="very_real_url.com",
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Changing the url so that it's clear that we're mocking

@freddyaboulton
Copy link
Copy Markdown
Collaborator Author

Thank you for the review @abidlabs ! Completely agree with your comments - I mocked out the remaining requests and applied the mock to the class. This is good for another review now!

@freddyaboulton freddyaboulton changed the title Fix Request Tests Mock out requests in TestRequest unit tests Jul 15, 2022
@abidlabs
Copy link
Copy Markdown
Member

Very clean, thanks for addressing the suggestions @freddyaboulton!

@omerXfaruq
Copy link
Copy Markdown
Contributor

omerXfaruq commented Jul 18, 2022

Sounds good! actually there is a similar library for this which I like, it mocks the request call and you don't mock your code at all. I would suggest we use it, but @freddyaboulton's approach works as well!

The library is aioresponses and there is a pytest plugin .

@freddyaboulton
Copy link
Copy Markdown
Collaborator Author

@farukozderim Thank you for bringing up that library. I'll file an issue to switch to that to get this merged in. I think there's lots of room to improve how we write tests (#1784)

@freddyaboulton freddyaboulton merged commit 4731b1a into main Jul 18, 2022
@freddyaboulton freddyaboulton deleted the fix-request-tests branch July 18, 2022 14:25
@freddyaboulton freddyaboulton mentioned this pull request Jul 19, 2022
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.

3 participants