Fix #525 FlushBufferTest #680
Conversation
Replace the disputed test (that tests poorly defined behaviour of a bad servlet) with two tests that check the accepted behaviour of a well written servlet. Once there is an accepted interpretation of the behaviour for the bad servlet, additional tests can be added to check that.
|
|
||
| try { | ||
| out.write(fill); | ||
| throw new IllegalStateException("write did not fail"); |
There was a problem hiding this comment.
Is this sufficient to trigger a test failure?
There was a problem hiding this comment.
It should be as the requirement is that the proceeding write closes the output stream. Wiring to a closed output stream should be an exception regardless of buffering.
There was a problem hiding this comment.
We could add an out.flush if you are really concerned, but I don't see it as necessary
There was a problem hiding this comment.
Sorry, should have been clearer. I'm not doubting the exception will be thrown. My question is whether the client will see any effects of the exception which will in turn triggered a test failure. Or to put it another way, will the client see the completed response and think all is well regardless of whether the exception is thrown on the server.
There was a problem hiding this comment.
In this test, the write a line 59 will complete the response and close the stream. The client will see that response as a 200. The subsequent write will throw an exception.... ah I see the problem, the client will not see the exception either way. Let me ponder a better approach.... hard in this test environment as everything is sent via the response.
There was a problem hiding this comment.
@olamy Can you test the latest version please
There was a problem hiding this comment.
@gregw all good with jetty 12.1.x
There was a problem hiding this comment.
@markt-asf How's this version? It does every test request twice with any bad behaviour after response completion of the first request stored in the session to be seen by the second request.
Replace the disputed test (that tests poorly defined behaviour of a bad servlet) with two tests that check the accepted behaviour of a well written servlet. Once there is an accepted interpretation of the behaviour for the bad servlet, additional tests can be added to check that.
|
OK for me to merge this? I've no idea what release it will end up in... but happy enough it will end up in some release some time. |
|
@gregw, go for it. I can then cherry-pick the |
* Fix #525 FlushBufferTest Replace the disputed test (that tests poorly defined behaviour of a bad servlet) with two tests that check the accepted behaviour of a well written servlet. Once there is an accepted interpretation of the behaviour for the bad servlet, additional tests can be added to check that.
Replace the disputed test (that tests poorly defined behaviour of a bad servlet) with two tests that check the accepted behaviour of a well written servlet.
Once there is an accepted interpretation of the behaviour for the bad servlet, additional tests can be added to check that.