Repository navigation
Conversation
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds validation for the relay_url configuration parameter to ensure it uses the ws:// or wss:// scheme, along with unit tests to verify this behavior. The review feedback points out a bug in the test suite where the invalid URL test case fails on the scheme check rather than the parsing check, and suggests using a truly unparseable URL. Additionally, the reviewer recommends replacing the custom substring search helper function with the standard library's strings.Contains function.
| name: "not a URL", | ||
| relayURL: "not-a-url", | ||
| wantErr: true, | ||
| errMsg: "relay_url must be a valid URL", | ||
| }, | ||
| } |
There was a problem hiding this comment.
The string "not-a-url" is successfully parsed by url.Parse as a relative path (with an empty scheme). As a result, it fails the scheme check (u.Scheme != "ws" && u.Scheme != "wss") rather than the url.Parse check, returning the error "relay_url must use ws:// or wss:// scheme..." instead of "relay_url must be a valid URL". This causes this test case to fail.
To properly test the url.Parse error path, use an actually unparseable URL like "ws://[::1".
{
name: "not a URL",
relayURL: "ws://[::1",
wantErr: true,
errMsg: "relay_url must be a valid URL",
},| import ( | ||
| "os" | ||
| "path/filepath" | ||
| "testing" | ||
| ) |
There was a problem hiding this comment.
Use the standard library's strings package instead of implementing a custom substring search function.
| import ( | |
| "os" | |
| "path/filepath" | |
| "testing" | |
| ) | |
| import ( | |
| "os" | |
| "path/filepath" | |
| "strings" | |
| "testing" | |
| ) |
| if tt.wantErr && tt.errMsg != "" { | ||
| if err == nil { | ||
| t.Errorf("Load() expected error containing %q, got nil", tt.errMsg) | ||
| } else if !containsString(err.Error(), tt.errMsg) { |
There was a problem hiding this comment.
Replace the custom containsString helper with strings.Contains from the standard library.
| } else if !containsString(err.Error(), tt.errMsg) { | |
| } else if !strings.Contains(err.Error(), tt.errMsg) { |
| func containsString(s, substr string) bool { | ||
| for i := 0; i <= len(s)-len(substr); i++ { | ||
| if s[i:i+len(substr)] == substr { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Remove the custom containsString helper function as it is no longer needed.
|
@Jah-yee can please sign CLA |
60b63d2 to
ff67cac
Compare
Validate that relay_url uses the ws:// or wss:// scheme in config.Load, and add table-driven tests covering the valid and invalid cases. The unparseable-URL test case uses "ws://[::1" to exercise the url.Parse error path, and "not-a-url" is covered separately as a missing-scheme case. The test uses strings.Contains instead of a custom substring helper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ff67cac to
0b29d16
Compare
|
Thanks for the review notes on
Verification: One open item remains: @blue4209211 asked me to sign the CLA, and the |
Validates relay_url scheme in Load() after unmarshaling, rejecting non-ws/wss schemes (https, http, ftp, etc.) with a clear error message.
Closes #91