persistent connection unit test #393

hs3180 · 2015-09-24T18:16:11Z

persistent connection feature(#306) and its integration test.

davvd · 2015-09-24T19:05:36Z

@hs3180 Thanks for your pull request, let me find someone who can review it

davvd · 2015-09-24T19:14:28Z

@dmzaytsev it's yours, please review

dmzaytsev · 2015-09-24T19:18:51Z

@hs3180 plese fix qulice issues

hs3180 · 2015-09-24T20:09:12Z

@dmzaytsev
I am sorry for that. The qulice issues was fixed.
But there are some strange results from Travis CI.
It failed for the first time https://travis-ci.org/yegor256/takes/jobs/82016143
And got time out for the second time https://travis-ci.org/yegor256/takes/jobs/82032807

pecko · 2015-09-24T20:14:32Z

@hs3180

But there are some strange results from Travis CI.
It failed for the first time https://travis-ci.org/yegor256/takes/jobs/82016143
And got time out for the second time https://travis-ci.org/yegor256/takes/jobs/82032807

Maybe related to jcabi/jcabi-github#1165

hs3180 · 2015-09-24T20:32:52Z

@pecko
thank you for your help.
I rebuild the project, and get the same result as my first try.
I believe that my patch leading some thing unstable into the project. I will go further to figure out the potential reason.

dmzaytsev · 2015-09-25T11:35:52Z

@hs3180 I would suggest you implement only unit tests here. Full implementation will be too big for one PR.
In my opinion it should be something like this

 new FtRemote(new TkText()).exec(
            new FtRemote.Script() {
                @Override
                public void exec(final URI home) throws IOException {

you have to parse home, extract host and port and do same as in your KeepAliveITCase
I think it should be unit tests for BkBasic.

Let's implement tests for various scenarios for a persistent connection:

the server returns or not returns 'Keep-Alive' header depending on the presence 'Keep-Alive' header in the request
the socket reused if 'Keep-Alive' header exists in the request
the socket closed if 'Keep-Alive' header not exists in the request
the socket closed if 'Keep-Alive' header exists in the request on timeout
another scenario?

@yegor256 are you agree?

dmzaytsev · 2015-09-25T11:36:40Z

I believe that my patch leading some thing unstable into the project. I will go further to figure out the potential reason.

@hs3180 I think you're right

yegor256 · 2015-09-25T14:58:40Z

@dmzaytsev yes, I agree. I'm in general in favor of small pull requests

hs3180 · 2015-09-26T08:32:15Z

@dmzaytsev
great suggestion.
according to http://tools.ietf.org/html/rfc2616#section-14.10, connection field is not required in HTTP 1.1(there is no MUST statement says that connection field is required).
according to http://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html,

A significant difference between HTTP/1.1 and earlier versions of HTTP is that persistent connections are the default behavior of any HTTP connection. That is, unless otherwise indicated, the client SHOULD assume that the server will maintain a persistent connection, even after error responses from the server.

so I think there are only three scenario to be tested:

connection is reused by default. establish a connection and handle two set of request-response by the same connection.
In order to remain persistent, all messages on the connection MUST have a self-defined message length (i.e., one not defined by closure of the connection), the server response 411 Length Required if there is no Content-Length field in headers and the connection is persistent.
a client that doesn't support persistent connection. connection: close must be given by client and the server should close the connection after response.

davvd · 2015-09-27T09:55:06Z

@ypshenychka review this ticket please, for compliance with our quality rules

ypshenychka · 2015-09-27T10:08:11Z

@hs3180 Why is this PR not merged? Have you created new PRs instead of this?

hs3180 · 2015-09-27T11:00:27Z

@ypshenychka
i've rollback my commits and ready to send a smaller pull request. i'm sorry for that github closed my PR after rollback automaticlly.

ypshenychka · 2015-09-28T05:35:27Z

@hs3180 I see, thanks.
Please correct your last message by indicating an addressee at the beginning.

ypshenychka · 2015-09-28T05:36:50Z

@dmzaytsev
According to our QA Rules:

The code reviewer found at least three problems in the code.

Only one issue was found during code review. Try to find at least three problems while future reviews.
Please confirm that everything is clear and you have no questions regarding this approach.

dmzaytsev · 2015-09-28T05:39:57Z

@ypshenychka I confirm

ypshenychka · 2015-09-28T07:43:18Z

@dmzaytsev Thanks.

hs3180 · 2015-09-29T07:17:04Z

@dmzaytsev
I've finished the unit tests described above. But it can't pass the CI obviously since the feature hasn't been implemented.

dmzaytsev · 2015-09-29T08:07:48Z

src/test/java/org/takes/http/BkBasicTest.java

@@ -60,9 +74,9 @@ public void handlesSocket() throws IOException {
        Mockito.when(socket.getInputStream()).thenReturn(
            new ByteArrayInputStream(
                Joiner.on("\r\n").join(
-                    "GET / HTTP/1.1",
+                    "POST / HTTP/1.1",


@hs3180 why did you change this line?

@dmzaytsev
In most practical applications, GET method shouldn't be attached with a body.
see latest discussion in #306 for more information.

dmzaytsev · 2015-09-29T08:08:04Z

src/test/java/org/takes/http/BkBasicTest.java

                    "Host:localhost",
-                    "Content-Length: 2",
+                    "Content-Length: 4",


@hs3180 and this one?

@dmzaytsev
The request body "hi" is actually "hi\r\n".

dmzaytsev · 2015-09-29T08:18:40Z

@hs3180 3 comments above

hs3180 · 2015-09-29T09:25:38Z

@dmzaytsev
updated for qulice and readability issue.

ypshenychka · 2015-09-29T10:55:12Z

@davvd Quality is acceptable.

davvd · 2015-09-29T12:15:12Z

@davvd Quality is acceptable.

@ypshenychka thanks for the QA review, we'll work better next time

dmzaytsev · 2015-09-29T12:29:09Z

@davvd @yegor256 this PR has not been reviewed yet

hs3180 · 2015-09-29T12:34:36Z

@davvd @ypshenychka
I'm sorry for closing this PR unexpectedly.
this PR is still updating and @dmzaytsev is reviewing now.

davvd · 2015-09-29T16:02:18Z

@davvd @yegor256 this PR has not been reviewed yet

@dmzaytsev OK, go ahead

davvd · 2015-09-29T16:02:19Z

@davvd @ypshenychka
I'm sorry for closing this PR unexpectedly.
this PR is still updating and @dmzaytsev is reviewing now.

@hs3180 OK, got it

dmzaytsev · 2015-09-29T18:16:49Z

src/test/java/org/takes/http/BkBasicTest.java

+        Mockito.when(socket.getPort()).thenReturn(0);
+        final ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        Mockito.when(socket.getOutputStream()).thenReturn(baos);
+        new BkBasic(new TkGreedy(new TkText(HELLO_WORLD))).accept(socket);


@hs3180 I still not understand how you check that the socket still open after first request...

dmzaytsev · 2015-09-29T18:21:16Z

@hs3180 one comment/question above
also please check out this article http://www.yegor256.com/2014/04/27/typical-mistakes-in-java-code.html#redundant-constants

Another typical mistake is to use constants in unit tests to avoid duplicate string/numeric literals in test methods. Don't do this! Every test method should work with its own set of input values.

dmzaytsev · 2015-10-02T11:05:55Z

@hs3180 ping

hs3180 · 2015-10-03T02:42:06Z

@dmzaytsev
thanks for your review. code updated.
I've moved persistent connection feature to a new wrapped Back named BkReuse

dmzaytsev · 2015-10-03T05:06:07Z

@hs3180 I don't see any unit test in this PR
I would suggest to start from tests

hs3180 · 2015-10-03T08:59:57Z

@dmzaytsev
so sorry, i've pushed wrong branch.
the unit tests was updated.

dmzaytsev · 2015-10-03T12:07:06Z

src/test/java/org/takes/http/BkReuseTest.java

+     * @throws IOException Socket IOException.
+     */
+    @Test
+    public void handleTwoRequestInOneConnection() throws IOException {


@hs3180 handlesTwoRequestInOneConnection
http://www.yegor256.com/2014/04/27/typical-mistakes-in-java-code.html?expand=1#test-method-names

same for javadoc and test name below

dmzaytsev · 2015-10-03T12:10:02Z

@hs3180 let's ignore broken tests and create puzzle to implement missed BkReuse
see http://www.yegor256.com/2009/03/04/pdd.html

dmzaytsev · 2015-10-05T15:01:23Z

@hs3180 ping

hs3180 · 2015-10-09T02:49:10Z

@dmzaytsev
I'm sorry, @davvd ask me to stop working with this issue #306.

davvd · 2015-10-10T12:51:12Z

@dmzaytsev I added 10 mins to @ypshenychka (for QA review) in transaction 66959064

paid, thanks, added 26 mins to your account, payment 66959076, 353 hours and 18 mins was spent

review comments (c=11) added as a bonus

+26 added to your rating, current score is: +1350

davvd added CR @dmzaytsev labels Sep 24, 2015

hs3180 closed this Sep 24, 2015

hs3180 reopened this Sep 24, 2015

hs3180 closed this Sep 24, 2015

hs3180 reopened this Sep 24, 2015

hs3180 changed the title ~~support persistent connection~~ persistent connection unit test Sep 26, 2015

hs3180 closed this Sep 26, 2015

hs3180 force-pushed the persistence branch from 41fa459 to 52ab389 Compare September 26, 2015 09:09

hs3180 reopened this Sep 29, 2015

dmzaytsev reviewed Sep 29, 2015
View reviewed changes

hs3180 mentioned this pull request Sep 29, 2015

implements persistent connection #397

Closed

dmzaytsev reviewed Sep 29, 2015
View reviewed changes

hs3180 force-pushed the persistence branch from 4061222 to 196a634 Compare October 3, 2015 02:38

yegor256#306 unit tests for persistent connection

bf84dfa

hs3180 force-pushed the persistence branch from 196a634 to bf84dfa Compare October 3, 2015 08:56

dmzaytsev reviewed Oct 3, 2015
View reviewed changes

hs3180 closed this Oct 9, 2015

hs3180 mentioned this pull request Oct 9, 2015

persistent connections #306

Closed

persistent connection unit test #393

persistent connection unit test #393

Conversation

hs3180 commented Sep 24, 2015

davvd commented Sep 24, 2015

davvd commented Sep 24, 2015

dmzaytsev commented Sep 24, 2015

hs3180 commented Sep 24, 2015

pecko commented Sep 24, 2015

hs3180 commented Sep 24, 2015

dmzaytsev commented Sep 25, 2015

dmzaytsev commented Sep 25, 2015

yegor256 commented Sep 25, 2015

hs3180 commented Sep 26, 2015

davvd commented Sep 27, 2015

ypshenychka commented Sep 27, 2015

hs3180 commented Sep 27, 2015

ypshenychka commented Sep 28, 2015

ypshenychka commented Sep 28, 2015

dmzaytsev commented Sep 28, 2015

ypshenychka commented Sep 28, 2015

hs3180 commented Sep 29, 2015

dmzaytsev Sep 29, 2015

Choose a reason for hiding this comment

hs3180 Sep 29, 2015

Choose a reason for hiding this comment

dmzaytsev Sep 29, 2015

Choose a reason for hiding this comment

hs3180 Sep 29, 2015

Choose a reason for hiding this comment

dmzaytsev commented Sep 29, 2015

hs3180 commented Sep 29, 2015

ypshenychka commented Sep 29, 2015

davvd commented Sep 29, 2015

dmzaytsev commented Sep 29, 2015

hs3180 commented Sep 29, 2015

davvd commented Sep 29, 2015

davvd commented Sep 29, 2015

dmzaytsev Sep 29, 2015

Choose a reason for hiding this comment

dmzaytsev commented Sep 29, 2015

dmzaytsev commented Oct 2, 2015

hs3180 commented Oct 3, 2015

dmzaytsev commented Oct 3, 2015

hs3180 commented Oct 3, 2015

dmzaytsev Oct 3, 2015

Choose a reason for hiding this comment

dmzaytsev commented Oct 3, 2015

dmzaytsev commented Oct 5, 2015

hs3180 commented Oct 9, 2015

davvd commented Oct 10, 2015