test: cover backend helpers#30
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds tests for permissions and port management, a processManifest helper in server.go, and extensive server unit and HTTP integration tests that cover token middleware, CORS, file I/O workers, upload handling, and related helpers. ChangesTest Coverage for Permissions, Port Management, and Server Components
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
|
Thanks for the contribution. The permissions and port manager tests are well-written and all pass. I checked the test coverage against the issue requirements. The PR covers most of the permissions and port manager deliverables, but the server tests are missing the bulk of what issue #14 asks for. Specifically, these server areas still need tests:
These are all listed in the issue under the Server Tests section. Without them, the server coverage is roughly 3% when the target is 60%. Could you take a pass at adding those? The existing test files in the PR show the right patterns to follow. |
PranavAgarkar07
added
gssoc:approved
|
Thanks for the detailed review. I took another pass at the server-side coverage and pushed commit 2d9b565 to this PR. Added coverage for:
Validation: git diff --check passes. I still cannot run go test ./beamsync/ in this local environment because the Go CLI is not installed, and Docker Desktop's daemon is not running for containerized Go validation. The GitHub workflow should be the source of truth for the actual Go test run. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
beamsync/server.go (1)
287-298: ⚡ Quick win
processManifest/manifestEntryduplicate the inline upload parsing and aren't wired into production.The
/uploadhandler still decodes the manifest with an inline anonymous struct (lines 665-678) using the same logic. As-is,processManifestandmanifestEntryare only exercised by tests and add no production value while diverging from the real parsing path. Reuse the helper in the handler so the tests actually cover the code path that runs in production.♻️ Wire the handler to the new helper
if formName == "beam_manifest" && filename == "" { - var manifest []struct { - Name string `json:"name"` - Size int64 `json:"size"` - } - if err := json.NewDecoder(part).Decode(&manifest); err == nil { - for _, f := range manifest { - fileSizes[f.Name] = f.Size - } - fmt.Printf("📦 Manifest received: %d files registered\n", len(manifest)) - } + if parsed, err := processManifest(part); err == nil { + for name, size := range parsed { + fileSizes[name] = size + } + fmt.Printf("📦 Manifest received: %d files registered\n", len(parsed)) + } part.Close() continue }🤖 Prompt for 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. In `@beamsync/server.go` around lines 287 - 298, The upload handler currently decodes the manifest using an inline anonymous struct instead of reusing processManifest/manifestEntry; update the /upload handler to call processManifest(r) (or pass the request body reader) and use the returned map[string]int64 instead of re-parsing with the inline struct, preserving the existing error handling and validation logic; remove or replace the inline anonymous manifest parsing so tests exercise processManifest/manifestEntry and ensure any variable names (e.g., fileSizes) and error paths in the handler are adjusted to use the helper's output.beamsync/server_test.go (1)
256-258: 💤 Low valueRedundant second
Shutdown().
server.Shutdown()here duplicates thedefer server.Shutdown()on line 222. The double call is harmless but adds no coverage; drop it (or remove the defer) to avoid confusion.🤖 Prompt for 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. In `@beamsync/server_test.go` around lines 256 - 258, The test currently calls server.Shutdown() twice (once via defer server.Shutdown() and again explicitly), so remove the redundant explicit call by deleting the final if err := server.Shutdown(); ... block; keep the deferred defer server.Shutdown() to ensure cleanup, or alternatively remove the defer and keep the explicit call—make sure only one server.Shutdown() invocation remains to avoid confusion.
🤖 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 `@beamsync/permissions_test.go`:
- Around line 5-17: The test TestDefaultTransferSettings currently asserts Mode,
MaxFileSizeMB, and BlockedExtensions but omits verifying TrustedDevices and
BlockedDevices; update TestDefaultTransferSettings to call
DefaultTransferSettings() and add assertions that len(settings.TrustedDevices)
== 0 and len(settings.BlockedDevices) == 0 (ensuring the default returns empty
slices), using t.Fatalf with a clear message similar to the existing checks.
In `@beamsync/port_manager_test.go`:
- Around line 8-25: The test TestFindAvailablePortSkipsBusyPorts creates a busy
listener on "127.0.0.1:0" while FindAvailablePort binds to ":port" (all
interfaces), so the busy socket doesn't actually block the binding; update the
test to bind the busy listener to the same address family used by
FindAvailablePort (e.g., use "0.0.0.0:0" or ":" instead of "127.0.0.1:0") so
busy.Addr().(*net.TCPAddr).Port yields a port that FindAvailablePort will
attempt to bind and thus correctly verifies port-skipping behavior for
FindAvailablePort.
- Around line 27-46: The test TestFindAvailablePortReportsExhaustion creates a
listener on "127.0.0.1:0" while FindAvailablePort binds to ":port", so the
addresses differ and the busy socket doesn't block the tested bind; change the
test's busy listener to bind to the same address family as FindAvailablePort
(use ":0" / all interfaces) so the busy port actually prevents FindAvailablePort
from acquiring the same port, keep capturing startPort from
busy.Addr().(*net.TCPAddr).Port and leave the rest of the test logic (calls to
FindAvailablePort, checks for error/zero port/nil listener) unchanged.
---
Nitpick comments:
In `@beamsync/server_test.go`:
- Around line 256-258: The test currently calls server.Shutdown() twice (once
via defer server.Shutdown() and again explicitly), so remove the redundant
explicit call by deleting the final if err := server.Shutdown(); ... block; keep
the deferred defer server.Shutdown() to ensure cleanup, or alternatively remove
the defer and keep the explicit call—make sure only one server.Shutdown()
invocation remains to avoid confusion.
In `@beamsync/server.go`:
- Around line 287-298: The upload handler currently decodes the manifest using
an inline anonymous struct instead of reusing processManifest/manifestEntry;
update the /upload handler to call processManifest(r) (or pass the request body
reader) and use the returned map[string]int64 instead of re-parsing with the
inline struct, preserving the existing error handling and validation logic;
remove or replace the inline anonymous manifest parsing so tests exercise
processManifest/manifestEntry and ensure any variable names (e.g., fileSizes)
and error paths in the handler are adjusted to use the helper's output.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2267abbb-3052-4d78-a8ca-0e90fe002e0c
📒 Files selected for processing (4)
beamsync/permissions_test.gobeamsync/port_manager_test.gobeamsync/server.gobeamsync/server_test.go
kunal-9090
force-pushed
the
codex/issue-14-backend-tests
branch
from
2d9b565 to
e0a706e
Compare
|
Follow-up: rebased this PR onto the latest upstream main and updated the new server tests to match the current stats/history-aware helper signatures. The latest Go and frontend checks CI run is passing now, along with CodeRabbit. |
|
This is looking really solid now @kunal-9090! 🔥 The test coverage is comprehensive — 534 lines across permissions, port manager, and server tests is a significant addition. I tested the merge locally and everything passes cleanly with zero race conditions. A few things I noticed while going through it:
None of these are blockers — the CI passes, coverage is way up, and these are easy follow-ups. Let me know if you want to address them before merge or we can ship and clean up after! Great work on this! 🚀 |
|
Thanks for the detailed review. I pushed a small follow-up that wires the upload manifest parsing through the existing processManifest helper, removes the redundant shutdown pattern in the lifecycle test, tightens the default permission assertions, and updates the port-manager tests to bind busy listeners on all interfaces.\n\nValidation: the PR's GitHub Go and frontend checks workflow passed on commit |
PranavAgarkar07
left a comment
PranavAgarkar07
left a comment
There was a problem hiding this comment.
Thank you for this thoughtful contribution, Kunal. The tests add real coverage to previously untested paths like writeFileToDisk, processManifest, copyChunked, and the worker pool error tolerance. These are the kinds of tests that catch production bugs rather than just padding coverage numbers.
The add/add conflicts with main were straightforward to resolve. Your additional device rule assertions in permissions_test.go, the ':0' bind address alignment with production in port_manager_test.go, and the removal of the redundant defer Shutdown() in server_test.go were all kept in the merge.
A few things worth considering for future contributions:
- Table-driven tests are the most idiomatic Go pattern for multi-case scenarios. The extension blocking and token middleware tests would benefit from them.
- Running tests with the -race flag would add confidence for the concurrent paths you tested.
- Using httptest.NewServer instead of a raw listener in test helpers aligns with established Go conventions.
None of these are merge blockers. The core logic is sound and the tests provide real value. Appreciate you taking this on.
|
Merged via 1fcb3cf. The conflicts with main were resolved locally by combining both sets of test additions. Thank you for contributing these, Kunal. The tests for writeFileToDisk, processManifest, copyChunked, and the worker pool are exactly the kind of coverage this codebase needed. |
PranavAgarkar07
added
type:docs
PranavAgarkar07
force-pushed
the
codex/issue-14-backend-tests
branch
from
09c0e8d to
1fcb3cf
Compare
…r tests Merging PR #42 which replaces PR #30 (closed due to merge workflow issue). Contributed by @kunal-9090. Closes #14.
|
Apologies for the workflow misstep. I originally merged this directly to main instead of via the PR, which closed it without a proper merge status. I then tried to reopen but GitHub did not allow reopening a closed PR with identical head and base refs. Since I cannot push to your fork branch, I created PR #42 with the exact same changes and merged it properly with the 'gssoc:approved', 'level:intermediate', and 'type:testing' labels. The commit 3444f84 on main includes your contributions. For GSSoC credit, could you please either enable maintainer edits on PR #30 so I can update the branch and merge it, or confirm that PR #42 works for tracking purposes on your end? |
|
Hey Kunal, I ran into a GitHub limitation with this PR -- since the branch state is locked it can not be reopened from this side. No worries though. Could you open a fresh pull request from your fork with any small change (even a README typo fix or a blank line)? I will merge it right away and apply the gssoc:approved label so you get GSSoC credit. The actual test code from PR 30 is already on main (landed via 3444f84), so the dummy PR is just for attribution. Thank you for the contribution. |
2 participants
Summary
Tests
go/gofmtnot on PATH).Closes #14
Summary by CodeRabbit
New Features
Tests