Skip to content

Add MockTcpServer, DaytimeTCPClient integration tests#168

Merged
garydgregory merged 10 commits intoapache:masterfrom
jkbkupczyk:mock_tcp_test_server_daytimetcpclient_tests
Feb 24, 2024
Merged

Add MockTcpServer, DaytimeTCPClient integration tests#168
garydgregory merged 10 commits intoapache:masterfrom
jkbkupczyk:mock_tcp_test_server_daytimetcpclient_tests

Conversation

@jkbkupczyk
Copy link
Contributor

  • 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

@garydgregory
Copy link
Member

@jkbkupczyk
Thank you for PR.
Please add a brief class level Javadoc to each new class.
Files should end in an empty line.

@jkbkupczyk
Copy link
Contributor Author

@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.

@jkbkupczyk
Copy link
Contributor Author

Hi @garydgregory, any new comments regarding this PR?


/**
* Gets the current value for maxTimeoutRetries
* Get the current value for maxTimeoutRetries
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't change the style of the Javadoc please. A getter "Gets...".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed that code by accident, and I already reverted it back. In pull #173 (that part) is fixed

@garydgregory
Copy link
Member

-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?

@jkbkupczyk
Copy link
Contributor Author

-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
master

mock_tcp_test_server_daytimetcpclient_tests
mock_tcp_test_server_daytimetcpclient_tests

did you check site/jacoco/org.apache.commons.net.daytime/DaytimeTCPClient.html?

@garydgregory garydgregory merged commit 2de5d64 into apache:master Feb 24, 2024
@garydgregory
Copy link
Member

@jkbkupczyk
Well, it works for me now locally. Merged! Sorry about the delay.

@jkbkupczyk jkbkupczyk deleted the mock_tcp_test_server_daytimetcpclient_tests branch August 19, 2025 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants