Repository navigation
test(desktop): the unreachable-server test keeps its port - #2020
Merged
Merged
Conversation
an_unreachable_server_is_reported_without_the_media_link bound a free port, released it, and expected nothing to answer there. Another test in the run could start an httpmock server on that port in between, and the request then reached it. The listener now lives for the test's whole life, with a thread that accepts each connection and drops it at once, so the request fails immediately and no timeout is involved. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Member
Author
|
Finding with no line in the diff. Standards · judgement (PR description): the checklist ticks "Added test to prevent regression", but the PR adds no test; it changes how an existing test holds its port. A ticked box that is false misleads a later reader. Fix: untick it and say the fix is the test itself. |
Review of #2020: the server in the test now accepts and hangs up, so the name says that rather than unreachable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mbeisser1
marked this pull request as ready for review
October 10, 2026 02:13
Member
Author
Fixed in the description: the box is unticked with "(the fix is the test itself)", and the description names the renamed test. Review of #2020, closed out.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug Description
Expected:
an_unreachable_server_is_reported_without_the_media_linkinsrc-tauri/src/commands/download.rs(renameda_server_that_hangs_up_is_reported_without_the_media_linkhere) passes on every run.Actual: it failed once in four full
src-tauriruns, when the request reached a live server.Root Cause
The test bound
127.0.0.1:0, read the port, and released the listener before the request. Seven other tests in the crate starthttpmockservers on ephemeral ports, so one of them could take the released port in between, and the request then reached that server instead of failing.Fix Description
The test keeps its listener for its whole life. A thread accepts each connection and drops it at once, so the request fails immediately with "Could not reach the server", the port is never free for another test, and no timeout is involved. The assertions are unchanged. Test-only.
How to Reproduce (Before Fix)
cargo test --manifest-path src-tauri/Cargo.tomlseveral times.httpmockserver takes the released port.How to Verify (After Fix)
cargo test --manifest-path src-tauri/Cargo.toml a_server_that_hangs_uppasses in about 0.03 s.src-taurisuite passes (206 tests).Impact Assessment
Regression Risk
None for the product: only a test changes. The accepting thread lives until the test process ends, which holds one port and one idle thread.
Checklist
Related Issues
Closes #2002.
🤖 Generated with Claude Code