Test that the response body is a ReadStream and not a string is missing

[essal0815]

(For a more detailed description, see post by @He3lixxx below.)

The MockRequests don't cover the whole flow. Normal Requests handled from a HttpService call on the HttpResponse writeOn:. The Mocks don't.

So the error that Response Body was a string and not ReadStream wasn't recognised.

We shall adjust the Mock so he also calls the writeOn:

[essal0815]

I've no idea of the mock structure yet. Also never wrote a mock object. My Estimation: 30

[skydivin4ng3l]

same for me 30

[richardebeling]

So, it I understand this correctly, this is the issue:

• Usually, the HttpService will at some point for a given HttpResponse call writeOn: aStream in order to be able to send the response back to the client.
• Our Applications are not involved in this at all, the closest we come is when we return the HttpResponse in processRequest. HttpService will then take over and do the network stuff.
• When we're testing, we're not having this flow, since we always use our RPTestHelper to make requests. This will end in request: aUrl on: anApplication method: aString headers: headers content: content at some point. This will create a MockRequest (which is a HttpRequest) and call processRequest with this request. It will return the HttpResponse that results from this request. In our tests, we always assert things about this response. For example, we do

  response := RPTestHelper get: '/index' on: app
  self assert: response body equals: 'Hello World'

  This does not call writeOn: for a stream since we don't need it.

Now, we could write one test that checks whether calling response writeOn: aStream works correctly, but I guess this was an error that would only occur for specific requests, so testing against one random request wouldn't really help.

We could also change the request:on:method:headers:content method so it calls response writeOn: aStream and then discards the result. This will trigger the error, but I don't really like it.

All in all, I think the problem is not that complex (each of these could be done in 5), but there is no easy and good way to really test this.

[schnaaabeltier]

20

[ninaihde]

20

[richardebeling]

@essal0815 @skydivin4ng3l @jakobmache @Nina510, please read through what I wrote. There's really not much we can do, we either to a hacky one-line fix:

We could also change the request:on:method:headers:content method so it calls response writeOn: aStream and then discards the result. This will trigger the error, but I don't really like it.

Or we do nothing. Either case, 20 is not realistic. Or does any of you have a different solution?

[essal0815]

I think a test for this would be fine. It wasn't that special requests I used. In the beginning it actually failed for each "real" request not maid by the TestHelper. After the first fix all the returns from handler where converted corectly, but I forgot, that there are also handlers which set the body directly... So I convert the body content each time to ReadStream before checking. (While writing I notice how inefficient it is always to convert before, the conversion would be totally fine in the false Branch...)

What ever, I think it is ok to write a test for, because there is only the one point, where the body is set for each handler...

I agree, that the estimation than is to high therefore. As far as I understand the HttpService of Squeak we would have to implement a MockSocketReadAndWrite. I think that is definitely to much for only having the call of one function tested.

My new estimation would be 5-8.

[richardebeling]

So I convert the body content each time to ReadStream before checking. (While writing I notice how inefficient it is always to convert before, the conversion would be totally fine in the false Branch...)

Did you see the readStream method? it's a noop on a ReadStream and a conversion on a string, so exactly what you would want.

Also, I think we're talking about two different things here:

• From the initial issue text, I was thinking this was about testing that the response body is a ReadStream. We can not practically test that since wrong code would usually be created by a user who adds a bad handler or bad middleware which sets the response body to a string. We don't have that code, so we can't test it. No Socket or Service or whatever Mock would be able to do that for us.
• Your last post sounds like you want to add automatic conversion to our code (which could then be tested, obviously). In this case, a user can not break it anymore. We don't need to do any mocks for this though, we just write one handler that sets the body to a string and check that it's a ReadStream afterwards.

If we want the second, which I'd like more, we should adapt the inital post to reflect that.

[richardebeling]

since the semester is over now, we will not fix this. Leaving the issue open for the next group working on this.