locustio / locust

Write scalable load tests in plain Python 🚗💨
MIT License
24.64k stars 2.96k forks source link

400: Bad Requests unable to be passed into catch_response #909

Closed DanielRussellkt closed 4 years ago

DanielRussellkt commented 5 years ago

Description of issue / feature request

when using client.get() with catch_response=true, the HTTPError is thrown, rather than being able to pass the request on to any subsequent code in the task.

For example:

from locust import HttpLocust, TaskSet, task

class MyTaskSet(TaskSet):
        @task(1)
            def oneYearTerm(self):
                with self.client.get("/myendpoint?param=someBadRequest", catch_response=True) as response:
                    if response.status_code != 400:
                        response.failure("Didn't detect bad response, got: " + str(response.status_code))

class MyLocust(HttpLocust):
    task_set = MyTaskSet
    min_wait = 0
    max_wait = 250
    host="https://my.url.co.uk/"

In the above, I would expect any exceptions to be caught such that I can test unhappy paths, such as where I have made an API respond with a 400, and perform any assertions on that bad response later on.

I currently do not have a better way in place outside of an ugly try/except, but even then I am not familiar with the locust codebase, and the documentation does not (As far as I could find) highlight where exceptions lie, so the actual HTTPException that was thrown I struggled to catch.

Expected behavior

Any exceptions from bad requests are caught when using catch_response=true, allowing me to operate on them in the subsequent code

Actual behavior

HTTPException is thrown: '400 Client Error: Bad Request for url'. This is returned by the API I am load testing, and until my tests say otherwise should be assumed to be a valid response, and be operable on

Environment settings (for bug reports)

Steps to reproduce (for bug reports)

Run a task with self.client.get(), with catch_response=true, against an endpoint or with data that provides a 400 Bad request. This will throw a HTTPException instead of allowing the response to be used as shown in the code example above.

heyman commented 4 years ago

Constructed a unit test for this, but I'm not able to reproduce the error. Do you know of any public URL that results in this error?

DanielRussellkt commented 4 years ago

Constructed a unit test for this, but I'm not able to reproduce the error. Do you know of any public URL that results in this error?

@cyberw this is reproducible with any endpoint that requires authorisation, spits out a bad request, or a forbidden. Heroku endpoints work for reproduction of this, and this definitely isn't invalid.

from locust import HttpLocust, TaskSet, task

class MyTaskSet(TaskSet):

    @task(1)
    def brokenRequestBadRequest(self):
        with self.client.get("/400", catch_response=True) as response:
            if response.status_code != 400:
                response.failure("Got unexpected response code: " + str(response.status_code) + " Error: " + str(response.text))
            else:
                print("400 Test passed")

    @task(1)
    def brokenRequestForbidden(self):
        with self.client.get("/403", catch_response=True) as response:
            if response.status_code != 403:
                response.failure("Got unexpected response code: " + str(response.status_code) + " Error: " + str(response.text))
            else:
                print("403 Test passed")

    @task(1)
    def brokenRequestUnauthorized(self):
        with self.client.get("/401", catch_response=True) as response:
            if response.status_code != 401:
                response.failure("Got unexpected response code: " + str(response.status_code) + " Error: " + str(response.text))
            else:
                print("401 Test passed")

class MyLocust(HttpLocust):
    task_set = MyTaskSet
    min_wait = 0
    max_wait = 0
    host = "http://the-internet.herokuapp.com/status_codes"

This exact test, copied and pasted, should show you the point I'm making, but definitely shouldn't be running load generation tools against public URLs.

The test does continue with the logic, printing the "4XX Test Passed" lines, therefore not ever failing:

[2020-02-04 15:00:19,264] a-pc-name/INFO/locust.main: Starting web monitor at *:8089
[2020-02-04 15:00:19,264] a-pc-name/INFO/locust.main: Starting Locust 0.9.0
[2020-02-04 15:00:23,214] a-pc-name/INFO/locust.runners: Hatching and swarming 1 clients at the rate 1 clients/s...
[2020-02-04 15:00:23,478] a-pc-name/INFO/stdout: 400 Test passed
[2020-02-04 15:00:23,479] a-pc-name/INFO/stdout: 
[2020-02-04 15:00:24,102] a-pc-name/INFO/stdout: 401 Test passed
[2020-02-04 15:00:24,102] a-pc-name/INFO/stdout: 
[2020-02-04 15:00:24,219] a-pc-name/INFO/locust.runners: All locusts hatched: MyLocust: 1
[2020-02-04 15:00:24,705] a-pc-name/INFO/stdout: 400 Test passed
[2020-02-04 15:00:24,706] a-pc-name/INFO/stdout: 
[2020-02-04 15:00:25,328] a-pc-name/INFO/stdout: 403 Test passed
[2020-02-04 15:00:25,328] a-pc-name/INFO/stdout: 
[2020-02-04 15:00:25,940] a-pc-name/INFO/stdout: 403 Test passed
[2020-02-04 15:00:25,940] a-pc-name/INFO/stdout: 
[2020-02-04 15:00:26,554] a-pc-name/INFO/stdout: 403 Test passed
[2020-02-04 15:00:26,554] a-pc-name/INFO/stdout: 
[2020-02-04 15:06:36,087] a-pc-name/INFO/locust.runners: Hatching and swarming 1 clients at the rate 1 clients/s...
[2020-02-04 15:06:36,297] a-pc-name/INFO/stdout: 403 Test passed
[2020-02-04 15:06:36,297] a-pc-name/INFO/stdout: 
[2020-02-04 15:06:37,017] a-pc-name/INFO/stdout: 403 Test passed
[2020-02-04 15:06:37,017] a-pc-name/INFO/stdout: 
[2020-02-04 15:06:37,091] a-pc-name/INFO/locust.runners: All locusts hatched: MyLocust: 1
[2020-02-04 15:06:37,646] a-pc-name/INFO/stdout: 401 Test passed
[2020-02-04 15:06:37,647] a-pc-name/INFO/stdout: 
[2020-02-04 15:06:38,250] a-pc-name/INFO/stdout: 401 Test passed
[2020-02-04 15:06:38,250] a-pc-name/INFO/stdout: 
[2020-02-04 15:06:38,860] a-pc-name/INFO/stdout: 400 Test passed
[2020-02-04 15:06:38,860] a-pc-name/INFO/stdout: 

However, these are still failed in the locust UI, and counted as fails (Presumably because of this HTTPError not being caught), therefore, a test intended to fail a login or send bad requests (or worse; Potential ReDOS attacks) cannot pass when a system successfully responds with a 400/401/403. The below screenshot shows that all these tests that can't fail as they are receiving the correct response are failing, and not with the response.failure message I've passed to them. This is definitely a bug:

Screenshot 2020-02-04 at 15 11 10

Also, I don't think failing requests are included in the total RPS in the top right or in the tables, was that intentionally designed that way or should I raise another issue?

EDIT: Digging into the code, even setting that print to: response.success("400 Test passed") Still fails the test, which I did not expect from this comment in the code:

        # if the user has already manually marked this response as failure or success
        # we can ignore the default haviour of letting the response code determine the outcome

So it obviously is just not catching the error from the client library and is failing on it.

tim-munn commented 4 years ago

Just to add this has been duplicated on multiple instances so not limited to one install

cyberw commented 4 years ago

Cool. Nobody had replied on the ticket for over a year so I closed it for lack of activity. Would anyone care to look at fixing it or at least creating a unit test (that doesnt rely on heroku)?

heyman commented 4 years ago

The problem is that if you don't explicitly mark the response as a success or failure (by calling success() or failure()) it'll use the default way of determining if the request was a success or a failure.

So you need to explicitly mark the response as a success to prevent them from being reported as a failure.

If you change the relevant parts of your example above to the following, it works:

with self.client.get("/401", catch_response=True) as response:
    if response.status_code != 401:
        response.failure("Got unexpected response code: " + str(response.status_code) + " Error: " + str(response.text))
    else:
        print("401 Test passed")
        response.success()
DanielRussellkt commented 4 years ago

@heyman, can confirm this works.

The question still kind of stands that, surely if the user wants to catch the response, and operate on it, then there shouldn't be assumed to be a default state of pass/fail based on HTTP codes?

heyman commented 4 years ago

The question still kind of stands that, surely if the user wants to catch the response, and operate on it, then there shouldn't be assumed to be a default state of pass/fail based on HTTP codes?

It could be a matter of taste, but I think it makes the most sense to use the default way of determining if a request was a success or a failure, if the request haven't been explicitly marked as success/failure.

Imagine the case where someone is testing a system that could return an error with the HTTP code still set to 200. In that case one might want to check the response content and call response.failure() if it contains an error message. However, one would still want the request to be marked as a failure if the HTTP status code was a 500. Similarly I can imagine a case where a specific HTTP error code could be considered a false negative, and one would want to mark that specific error code as a success, but that wouldn't mean that I automatically would want to consider all HTTP status codes a success.

DanielRussellkt commented 4 years ago

@heyman I agree with this, and that would ultimately be up to the test writer to get it correct, but my main issue with that solution is how its reported, particularly in response.failure cases.

The reporting of the default HTTP error from the requests library seems to overwrite any failure message you want to use to group different fails from different scenarios in the reporting; for example that screenshot in my first comment is printing out the error raised, rather than the one in my failure message (Which in this case isn't too informative, but it could be). This means that any fails due to 4XX errors for the same endpoint would be grouped by error in the reporting, for example if three or four different tasks for the same endpoint failed due to 400 bad requests, you'd not know easily which one it is from the reporting, and may have to come back to fix the tests multiple times, and would find it really hard to identify a genuine error in the system because of the uninformative default responses.

heyman commented 4 years ago

Maybe I'm misunderstanding, but I'm not able to reproduce the issue.

I changed one of the tasks in your example to:

    @task(1)
    def brokenRequestForbidden(self):
        with self.client.get("/403", catch_response=True) as response:
            if response.status_code != 403:
                response.failure("Got unexpected response code: " + str(response.status_code) + " Error: " + str(response.text))
            else:
                import random
                if random.randint(0,2) == 0:
                    response.failure("CUSTOM MESSAGE")
                print("403 Test passed")

And this is the failure table I get:

image

DanielRussellkt commented 4 years ago

I think I must have got something wrong in my own test, as I can't reproduce it with the one I commented on here either actually. Thanks, seems like there's nothing wrong.