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

FIX REST DELETE action. #47

Closed
wants to merge 1 commit into from
Closed

FIX REST DELETE action. #47

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 28, 2020

REST DELETE actions doe not provide content. So there is no 'Content-Type' header available. So use None. Fix #46 (#46)

REST DELETE actions doe not provide content. So there is no 'Content-Type' header available. So use None. Fix #46 (#46)
@kumar303
Copy link
Owner

@JoshuaR1337 ah, right, this looks like the right thing to do. Can you add tests for it? https://github.com/kumar303/hawkrest/tree/620610733965941512c82f50852e0079df3bdb43/tests

@ghost
Copy link
Author

ghost commented Jan 27, 2021

Hi, I am sorry, but I do not know how to add a test command for this? There should be already a test for this? Else there was no test at all. And there has not been made any tests for 3 years now...

So when I changed the code as proposed here, and rerun the testing as is, nothing changed. Therefore, I say that this will not break existing functionality.

And as far I can judge, there are no tests for POST, PUT, and DELETE actions....

@kumar303
Copy link
Owner

I think you should be able to copy this test and change the setup to self.request(method='DELETE', content_type=None), maybe?

So when I changed the code as proposed here, and rerun the testing as is, nothing changed. Therefore, I say that this will not break existing functionality.

I'm not too worried about it breaking existing functionality but I don't want to break this new functionality in the future on accident. Thus, it needs some test coverage. In other words, removing the line you added should break a test in the suite.

@kumar303
Copy link
Owner

Also, here are instructions for how to run the test suite: https://hawkrest.readthedocs.io/en/latest/developers.html

Thanks for your interest in fixing this issue!

@ghost
Copy link
Author

ghost commented Jan 28, 2021

I am sorry to say, but I think this is not my job. As the tests are more then 3 year old, and they are all failing for python3, you cannot expect from me to fix all this.

The page https://hawkrest.readthedocs.io/en/latest/developers.html is NOT describing how to make tests. Running tox will produce a tons of errors, due to old testing code for invalid python versions.

So, I think you cannot ask me to add tests for a system that I do not know. I have no knowledge of TOX.

Your suggestion does not do anything. Because I will not get the error when making a DELETE request with the existing code. So I would expect errors when running the tests, which is not the case.
Therefore I state this test scripts are not correct. And you cannot ask me to fix that!

Added to: test_middleware.py

def test_respond_ok_delete(self):
        req, sender = self.request(method='DELETE', content_type='application/json')

        response = HttpResponse('the response')
        res = self.mw.process_response(req, response)
        self.accept_response(res, sender)

and to test_authentication.py:

def test_hawk_delete(self):
        post_data = ''
        content_type = 'application/json'
        method = 'DELETE'
        sender = self._sender(content=post_data,
                              content_type=content_type,
                              method=method)
        req = self._request(sender,
                            content_type=content_type,
                            data=post_data,
                            method=method)

        assert isinstance(self.auth.authenticate(req)[0],
                          HawkAuthenticatedUser), (
            'Expected a successful authentication returning a user')

For both, using a content type of None, will break the testing itself, as the testing code does not check this, and always expect a string for content-type.

Again, I state the current testing code is not correct, outdated, and broken you can not expect from me to fix this all. Please pull this request... I have spent way more time about this discussion and stupid tests than it took the fix the code....

@kumar303
Copy link
Owner

kumar303 commented Jan 28, 2021

I understand that you had trouble getting the tests to run and that it was frustrating. Yes, sometimes it takes longer to write regression tests but this is the only way to make sure things work in the future.

This command will run the tests only on Python 2.7 (if you have it installed). Do these tests pass for you?

tox -e py27-django1.8-drf3.4

(I think the docs do need updating for this command, sorry.)

If you don't have Python 2.7, these other commands are currently defined in tox.ini:

tox -e py34-django1.8-drf3.5
tox -e py35-django1.8-drf3.5

If you can get one of them to run then maybe it would help you create new tests.

And, of course, I wouldn't expect you to fix anything that's currently broken. Thanks for spending time on fixing this bug.

@ghost ghost closed this by deleting the head repository Nov 22, 2022
This pull request was closed.
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.

Error when there is no content-type available (e.g. DELETE requests).
1 participant