Add companion app to fully test sockets in unit tests#408
Conversation
- Add .NET 10 console app. - Add documentation for companion app. - Add socket round trip unit tests. Assisted by Claude Sonnet 4.6
nfbot
added
Type: enhancement
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: nanoframework/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds a host-side Network Test Companion with TCP/UDP echo services and firewall helpers, configures socket tests to connect to it, and introduces TCP/UDP round-trip coverage for device and emulator scenarios. ChangesNetwork Test Companion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SocketRoundTripTests
participant CompanionClient
participant CommandServer
participant TcpEchoServer
SocketRoundTripTests->>CompanionClient: StartTcpEcho(port)
CompanionClient->>CommandServer: Send JSON command
CommandServer->>TcpEchoServer: Start echo listener
CommandServer-->>CompanionClient: Return JSON status
SocketRoundTripTests->>TcpEchoServer: Send test payload
TcpEchoServer-->>SocketRoundTripTests: Echo payload
Suggested labels: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Previously published | This build | |
|---|---|---|
| Native version | 100.2.0.12 |
100.2.0.14 |
| Checksum | 0x6DFA71D6 |
0x8EFB9944 |
👉 Tracking issue: https://github.com/nanoframework/Home/issues/1816
The issue contains a link to the stubs artifact from this build and instructions for GitHub Copilot to perform the update.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Tests/NetworkTestCompanion/FirewallHelper.cs`:
- Around line 80-81: Update RemoveWindows to accept the TCP and UDP port lists
used by SetupWindows, then delete each rule using the complete
nF-TestCompanion-TCP-{port} and nF-TestCompanion-UDP-{port} names instead of
only RulePrefix; keep the success message only after all deletions complete.
- Around line 115-142: IsUfwActive() can incorrectly return false when ufw
status requires elevated permissions, suppressing necessary Linux port-opening
guidance. Update the Linux guidance logic around the caller of IsUfwActive() to
print the ufw instructions unconditionally, or replace the check with a
permission-safe detection method; avoid using IsUfwActive() as a gate for
displaying those commands.
In `@Tests/README.md`:
- Around line 84-88: Update the firewall examples in Tests/README.md to use the
actual round-trip test ports 7001–7004 instead of port 7, including both the ufw
and iptables/nftables sections; alternatively, clearly label port 7 as an
example and identify the test command’s ports as the required values.
- Line 13: The Tests README table incorrectly labels round-trip tests as future
work. Update the SocketTests row to state that round-trip tests are present, and
mention their companion requirement as described, keeping it consistent with
SocketRoundTripTests.
In `@Tests/SocketTests/CompanionClient.cs`:
- Around line 26-29: The companion socket initialization in the CompanionClient
constructor should configure a short ReceiveTimeout before connecting, so
stalled reads fail promptly instead of waiting for the session timeout.
Optionally set SendTimeout as well to guard writes, using the existing _socket
setup.
- Around line 75-78: The response-reading logic in the companion client assumes
one Socket.Receive call returns the complete message and uses an unnecessary
fixed sleep. Update the code around the Receive call to repeatedly read into the
buffer, accumulate bytes, and stop when a newline is received or the socket
reaches EOF; remove the Thread.Sleep dependency while preserving correct JSON
response handling.
In `@Tests/SocketTests/SocketRoundTripTests.cs`:
- Around line 201-212: Update GetLocalIp() to skip loopback addresses,
disconnected interfaces, and virtual adapters before selecting an IPv4 address;
retain the existing fallback to "0.0.0.0" when no suitable interface is found.
- Around line 69-84: Ensure socket resources are cleaned up on both success and
failure paths: wrap each echo test’s `companion.Stop(echoPort)` in a finally
block surrounding the client/send/receive assertions, and update the
server/accepted-socket test to use using declarations or try/finally so both are
disposed even when assertions or setup throw. Apply this to the test methods
covering the referenced round-trip scenarios and the server/accepted socket
flow.
- Around line 146-156: Configure a finite ReceiveTimeout on the sockets used by
the round-trip tests before any blocking reads. Set it on the TCP client sockets
before their receive loops and on the UDP client in the test using ReceiveFrom,
such as the socket created in the UDP setup, so lost echoes or stalled peers
raise SocketException instead of blocking indefinitely.
In `@Tests/SocketTests/SocketTests.nfproj`:
- Around line 65-111: Update the comment above GenerateTestConfiguration to
accurately state that TestConfiguration.cs is generated during the build and is
not committed to source, rather than claiming it is committed. Clarify that a
fresh checkout receives the file when the target runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: nanoframework/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8e93dc5a-3547-4a32-8686-4bd2f10acffd
📒 Files selected for processing (17)
.gitignore.runsettingsTests/NetworkTestCompanion/CommandServer.csTests/NetworkTestCompanion/FirewallHelper.csTests/NetworkTestCompanion/NetworkTestCompanion.csprojTests/NetworkTestCompanion/Program.csTests/NetworkTestCompanion/TcpEchoServer.csTests/NetworkTestCompanion/UdpEchoServer.csTests/README.mdTests/SocketTests/CompanionClient.csTests/SocketTests/SocketPair.csTests/SocketTests/SocketRoundTripTests.csTests/SocketTests/SocketTests.nfprojTests/SocketTests/SocketTools.csTests/SocketTests/packages.configTests/SocketTests/packages.lock.jsonnanoFramework.System.Net.sln
***NO_CI*** (cherry picked from commit 25d2755)
2 participants
Description
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist: