Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More intuitive http response assertions #86

Closed
epicserve opened this issue Jan 14, 2019 · 15 comments
Closed

More intuitive http response assertions #86

epicserve opened this issue Jan 14, 2019 · 15 comments

Comments

@epicserve
Copy link
Contributor

I've been thinking it would be better to make assert methods for http status codes instead of the current response_XXX pattern.

Rational:

  • The camel case and assert prefix match the Python TestCase pattern.
  • Remembering a name instead of a number is easier.
  • It makes your code more human readable.

For example we could add the following:

class BaseTestCase(object):

    def assertHttpStatus(self, status_code: int, response: object = None):
        response = self._which_response(response)
        self.assertEqual(response.status_code, status_code)

    def assertHttpOk(self, response=None):
        self.assertHttpStatus(200, response)

    def assertHttpRedirects(self, response: object = None, url: str = None):
        self.assertHttpStatus(302, response)
        response = self._which_response(response)
        if url is not None:
            self.assertEqual(response.url, url)

    def asssertHttpForbidden(self, response: object = None):
        self.assertHttpStatus(403, response)

    def asssertHttpNotFound(self, response: object = None):
        self.assertHttpStatus(404, response)

    def asssertHttpBadRequest(self, response: object = None):
        self.assertHttpStatus(403, response)
@frankwiles
Copy link
Member

I like this idea as long as we keep the ones we have for backward compatibility. That way we can have the best of both worlds (I tend to think in the numbers not names, but I'm sure I'm probably in the minority there)

@epicserve
Copy link
Contributor Author

@frankwiles, I think it's cool to keep the old ones around. If the project was new I probably would want to change them to assertHttp200, assertHttp302, etc. for consistency and to match the test case pattern.

At some point it might be worth considering deprecating and then removing old methods as some point.

Just "thinking out loud" :)

@epicserve
Copy link
Contributor Author

Want me to contribute a PR?

@frankwiles
Copy link
Member

Yep that would be great!

@epicserve
Copy link
Contributor Author

@frankwiles,

Added a PR, let me know what you think.

@jefftriplett
Copy link
Member

jefftriplett commented Jul 31, 2019

We were talking about this earlier and are struggling with having 2 or 3 names for the same methods. I think filling in any missing status codes is valid, but I'm not sure how to resolve us wanting to keep backward compatibility while trying to keep the API clean.

Moving from response_200 to assertHttpOk is hard for me to wrap my dyslexic brain around. I'm going to have to look it up everything I reach a new status code. The assertHttp200 methods are a bit easier to remember, but I'm also not a fan of camel case just for the sake of unit tests roots originating from Java.

@epicserve
Copy link
Contributor Author

@jefftriplett and @frankwiles,

What about keeping all the existing assert methods and then going forward use a hybrid solution that the Django Rest Framework uses where they have both the error code number and a slug of what the status code is.

The new assert methods would like the following?

def assert_http_200_ok()
def assert_http_403_forbidden()
def assert_http_404_not_found()
def assert_http_500_internal_server_error()

The benefit would be that if you're using an IDE you could find the method by either the number or name. This also has the added benefit of helping those that are new to the status codes of learning the number and what the meaning is behind the number.

@jefftriplett
Copy link
Member

@epicserve I like that idea a lot. It fits my brain better, and we still have to have backward compatibility either way. I'll defer to the others to see what they think.

@frankwiles
Copy link
Member

yeah if we can keep response_200 and just add in assert_http_200_ok as an option I'm all for it!

@epicserve
Copy link
Contributor Author

@frankwiles and @jefftriplett,

Do you want me to make a PR based on what has been outlined?

@epicserve
Copy link
Contributor Author

@frankwiles and @jefftriplett,

I went ahead and created a new PR. If it looks good, let me know and I can update the README as well.

@epicserve
Copy link
Contributor Author

@frankwiles,

I created a PR for the readme updates. Once merged, we can close this issue.

@epicserve
Copy link
Contributor Author

@jefftriplett or @frankwiles,

Any change we could get the readme changes merged in and a release made? I have a project I would like to update with the changes made in this issue.

@frankwiles
Copy link
Member

Done! I'm going to add a ticket for myself to re-add in the older response_200 style to the docs as I personally prefer them and while I think many users will like yours better, I don't really intend to deprecate them as much as keep both in parallel.

@epicserve
Copy link
Contributor Author

@frankwiles,

Thank you for merging that in. I sort of wish now that you would have said you didn't want this contribution because I'm not a fan of having two ways to do things. That feels less pythonic and seems to go against, "There should be one-- and preferably only one --obvious way to do it."

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

No branches or pull requests

3 participants