You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "jkbkupczyk (via GitHub)" <gi...@apache.org> on 2023/07/27 21:41:22 UTC
[GitHub] [commons-net] jkbkupczyk opened a new pull request, #168: Add MockTcpServer, DaytimeTCPClient integration tests
jkbkupczyk opened a new pull request, #168:
URL: https://github.com/apache/commons-net/pull/168
- Adds MockTcpServer - base class for all other TCP Server impls, e.g. we can have _MockDiscardTCPServer_ to test _DiscardTCPClient_ or _MockEchoTCPServer_ to test _EchoTCPClient_
- Adds MockDaytimeTCPServer that uses the given clock to mock returned daytime string
- Adds DaytimeTCPClient integration tests
FYI
@garydgregory @kinow @sebbASF
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add MockTcpServer, DaytimeTCPClient integration tests [commons-net]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #168:
URL: https://github.com/apache/commons-net/pull/168#issuecomment-1962419822
@jkbkupczyk
Well, it works for me now locally. Merged! Sorry about the delay.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-net] jkbkupczyk commented on a diff in pull request #168: Add MockTcpServer, DaytimeTCPClient integration tests
Posted by "jkbkupczyk (via GitHub)" <gi...@apache.org>.
jkbkupczyk commented on code in PR #168:
URL: https://github.com/apache/commons-net/pull/168#discussion_r1292369555
##########
src/test/java/org/apache/commons/net/tftp/TFTPServer.java:
##########
@@ -610,7 +610,7 @@ protected void finalize() throws Throwable {
}
/**
- * Gets the current value for maxTimeoutRetries
+ * Get the current value for maxTimeoutRetries
Review Comment:
I pushed that code by accident, and I already reverted it back. In pull #173 [(that part)](https://github.com/apache/commons-net/pull/173/files#diff-8cfe322813dd0689aac3ae713d40911dfba80a16e7bcad4f6a95d1106e4da5baR613) is fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-net] jkbkupczyk commented on pull request #168: Add MockTcpServer, DaytimeTCPClient integration tests
Posted by "jkbkupczyk (via GitHub)" <gi...@apache.org>.
jkbkupczyk commented on PR #168:
URL: https://github.com/apache/commons-net/pull/168#issuecomment-1655188260
> @jkbkupczyk Thank you for PR. Please add a brief class level Javadoc to each new class. Files should end in an empty line.
Thank you @garydgregory for review :)
I added missing _Jacadocs_ and rebased onto master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-net] jkbkupczyk commented on pull request #168: Add MockTcpServer, DaytimeTCPClient integration tests
Posted by "jkbkupczyk (via GitHub)" <gi...@apache.org>.
jkbkupczyk commented on PR #168:
URL: https://github.com/apache/commons-net/pull/168#issuecomment-1666572502
Hi @garydgregory, any new comments regarding this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-net] garydgregory commented on a diff in pull request #168: Add MockTcpServer, DaytimeTCPClient integration tests
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #168:
URL: https://github.com/apache/commons-net/pull/168#discussion_r1292352852
##########
src/test/java/org/apache/commons/net/tftp/TFTPServer.java:
##########
@@ -610,7 +610,7 @@ protected void finalize() throws Throwable {
}
/**
- * Gets the current value for maxTimeoutRetries
+ * Get the current value for maxTimeoutRetries
Review Comment:
Don't change the style of the Javadoc please. A getter "Gets...".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add MockTcpServer, DaytimeTCPClient integration tests [commons-net]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #168:
URL: https://github.com/apache/commons-net/pull/168
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-net] garydgregory commented on pull request #168: Add MockTcpServer, DaytimeTCPClient integration tests
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #168:
URL: https://github.com/apache/commons-net/pull/168#issuecomment-1654637377
@jkbkupczyk
Thank you for PR.
Please add a brief class level Javadoc to each new class.
Files should end in an empty line.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-net] jkbkupczyk commented on pull request #168: Add MockTcpServer, DaytimeTCPClient integration tests
Posted by "jkbkupczyk (via GitHub)" <gi...@apache.org>.
jkbkupczyk commented on PR #168:
URL: https://github.com/apache/commons-net/pull/168#issuecomment-1722255070
> -1 as the PR stands today: The PR does not test anything new that isn't already tested. Before and after this PR, the code coverage is the same at: 40% instructions / 31% branches. Run `mvn clean site -Pjacoco` then open `target/site/jacoco/index.html` Or am I missing something?
Hi, @garydgregory
I ran `mvn clean site -Pjacoco` both on `master` and `mock_tcp_test_server_daytimetcpclient_tests` and these are the results I've got:
**master**
**mock_tcp_test_server_daytimetcpclient_tests**
did you check `site/jacoco/org.apache.commons.net.daytime/DaytimeTCPClient.html`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-net] garydgregory commented on pull request #168: Add MockTcpServer, DaytimeTCPClient integration tests
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #168:
URL: https://github.com/apache/commons-net/pull/168#issuecomment-1715660795
-1 as the PR stands today: The PR does not test anything new that isn't already tested.
Before and after this PR, the code coverage is the same at: 40% instructions / 31% branches.
Run `mvn clean site -Pjacoco` then open `target/site/jacoco/index.html`
Or am I missing something?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org