[e2e] add tests #26
Merged
[e2e] add tests #26
Conversation
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
🤖 Pull request artifacts
|
WalkthroughAdds GitHub Actions e2e-test job; tightens URL validation for the webhooks register command; and introduces an E2E Go module, test harness (builds CLI), mock-server test utility, and comprehensive end-to-end test suites for general, messages, and webhooks CLI flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant CLI as smsgate (webhooks register)
participant V as URL Validator
participant C as HTTP Client
participant S as ASG Endpoint
U->>CLI: smsgate webhooks register --url {URL} --event {EVENT} [--id]
CLI->>V: Trim & parse URL
alt invalid URL or scheme
V-->>CLI: Error ("invalid URL")
CLI-->>U: Exit (ParamsError) / print error
else valid URL
V-->>CLI: OK (trimmed URL)
CLI->>C: POST /webhooks {id?, url, event}
C->>S: HTTP POST
S-->>C: JSON 2xx / 4xx
C-->>CLI: Response
CLI-->>U: Print result / error
end
sequenceDiagram
autonumber
participant CI as GitHub Actions (e2e-test)
participant Builder as go build (repo root)
participant TestBin as ./smsgate
participant Mock as Mock HTTP Server
participant TestRunner as go test
CI->>Builder: go build -o tests/e2e/smsgate (cmd.Dir = ../../)
Builder-->>CI: Binary tests/e2e/smsgate
CI->>Mock: Start mock server(s) via CreateMockServer
CI->>TestRunner: go test ./tests/e2e -v
TestRunner->>TestBin: Execute with env (ASG_ENDPOINT, creds)
TestBin->>Mock: HTTP interactions (POST/GET /messages, /webhooks)
Mock-->>TestBin: Controlled responses
TestBin-->>TestRunner: stdout/stderr results
TestRunner-->>CI: Test results
CI->>Builder: Cleanup binary
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks (2 warnings, 1 inconclusive)❌ Failed checks (2 warnings, 1 inconclusive)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
internal/commands/webhooks/register.go (2)
49-51: Harden URL validation (accept uppercase schemes, reject malformed/relative URLs).Prefix checks miss valid inputs like "HTTPS://…" and allow malformed URLs. Parse instead and check scheme/host; keep the same error text to preserve your E2E assertions.
Apply:
@@ - if !strings.HasPrefix(url, "http://") && !strings.HasPrefix(url, "https://") { - return cli.Exit("invalid URL", codes.ParamsError) - } + // accept only absolute http/https URLs + u := strings.TrimSpace(url) + parsed, err := url.Parse(u) + if err != nil || parsed.Scheme == "" || parsed.Host == "" || (parsed.Scheme != "http" && parsed.Scheme != "https") { + return cli.Exit("invalid URL", codes.ParamsError) + }Note: add the import in this file:
-import ( - "fmt" - "strings" +import ( + "fmt" + "net/url" + "strings"
45-47: Optional: trim whitespace before validation.Prevents " URL" from passing emptiness check but failing parse for the wrong reason.
- url := c.Args().Get(0) + url := strings.TrimSpace(c.Args().Get(0)).github/workflows/go.yml (1)
71-96: Speed up E2E job with module/cache reuse.Cache Go modules to reduce CI time; also set working-directory to simplify steps.
e2e-test: name: E2E Test runs-on: ubuntu-latest needs: test steps: - name: Checkout code into workspace directory uses: actions/checkout@v4 + - name: Cache Go modules + uses: actions/cache@v4 + with: + path: | + ~/go/pkg/mod + ~/.cache/go-build + key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }} + restore-keys: | + ${{ runner.os }}-go- + - name: Set up Go uses: actions/setup-go@v5 with: go-version: stable @@ - - name: Install E2E test dependencies - run: cd tests/e2e && go mod download + - name: Install E2E test dependencies + working-directory: tests/e2e + run: go mod download @@ - - name: Run E2E tests - run: cd tests/e2e && go test -v ./... + - name: Run E2E tests + working-directory: tests/e2e + run: go test -v ./...tests/e2e/general_test.go (1)
12-13: Fix misleading comment.This test calls the help flag, not the send command.
- // Run the CLI binary with the send command + // Run the CLI binary with the --help flagtests/e2e/messages_test.go (3)
33-41: Prefer reserved test numbers to avoid real PII.Swap numbers for examples like +12025550123, +12025550124, etc.
- phones: []string{"+19162255887"}, + phones: []string{"+12025550123"}, @@ - phones: []string{"+19162255887", "+1234567890", "+19876543210"}, + phones: []string{"+12025550123", "+12025550124", "+12025550125"}, @@ - phones: []string{"+19162255887"}, + phones: []string{"+12025550123"}, @@ - phones: []string{"+19162255887"}, + phones: []string{"+12025550123"}, @@ - phones: []string{"+19162255887"}, + phones: []string{"+12025550123"},Also applies to: 45-55, 57-67, 69-91, 93-104
186-191: Consider asserting structured output for stability.Optional: add
--format jsonfor send and setexpectJSON=truein relevant cases to assert response shape instead of text.Example change in args:
- args := []string{ - "send", + args := []string{ + "--format", "json", + "send",And set
expectJSON: truein the corresponding table entries.Also applies to: 155-175
335-368: Use exact error matching where possible.For 404 vs 200 branches, consider asserting full message (or JSON field) to prevent brittle substring matches.
tests/e2e/webhooks_test.go (2)
98-100: Remove debug prints to keep test output clean.- fmt.Println(stdout.String()) - fmt.Println(stderr.String())
295-303: Avoid asserting path when webhook ID is empty.In the “empty id” case the CLI should fail before issuing an HTTP call; asserting
"/webhooks/"+tt.webhookIDwith an empty ID can be misleading.- assert.Equal(t, http.MethodDelete, r.Method) - assert.Equal(t, "/webhooks/"+tt.webhookID, r.URL.Path) + assert.Equal(t, http.MethodDelete, r.Method) + if tt.webhookID != "" { + assert.Equal(t, "/webhooks/"+tt.webhookID, r.URL.Path) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/e2e/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
.github/workflows/go.yml(1 hunks)internal/commands/webhooks/register.go(1 hunks)tests/e2e/general_test.go(1 hunks)tests/e2e/go.mod(1 hunks)tests/e2e/main_test.go(1 hunks)tests/e2e/messages_test.go(1 hunks)tests/e2e/testutils/testutils.go(1 hunks)tests/e2e/webhooks_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/e2e/webhooks_test.go (2)
tests/e2e/testutils/testutils.go (1)
CreateMockServer(8-10)cmd/smsgate/smsgate.go (1)
main(22-100)
tests/e2e/general_test.go (1)
cmd/smsgate/smsgate.go (1)
main(22-100)
tests/e2e/main_test.go (1)
cmd/smsgate/smsgate.go (1)
main(22-100)
tests/e2e/messages_test.go (2)
tests/e2e/testutils/testutils.go (1)
CreateMockServer(8-10)cmd/smsgate/smsgate.go (1)
main(22-100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: goreleaser
🔇 Additional comments (2)
tests/e2e/testutils/testutils.go (1)
8-10: LGTM — simple, focused helper.tests/e2e/webhooks_test.go (1)
116-152: Good coverage of invalid register paths.Covers empty URL, bad scheme, missing/invalid event and matches CLI messages.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e/general_test.go (1)
3-9: Harden the e2e help test (timeout, stderr check, less brittle match).Prevents hangs, catches unexpected stderr, and avoids Unicode brittleness in the assertion.
import ( + "context" "bytes" "os/exec" "testing" + "time" "github.com/stretchr/testify/assert" ) @@ - cmd := exec.Command("./smsgate", "--help") + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, "./smsgate", "--help") cmd.Stdout = &stdout cmd.Stderr = &stderr @@ - err := cmd.Run() + err := cmd.Run() + if ctx.Err() == context.DeadlineExceeded { + t.Fatalf("smsgate --help timed out: stderr=%s", stderr.String()) + } @@ - // Verify the output - assert.Contains(t, stdout.String(), "CLI interface for working with SMS Gateway for Android™") + // Verify the output + assert.Empty(t, stderr.String(), "unexpected stderr output") + assert.Contains(t, stdout.String(), "CLI interface for working with SMS Gateway for Android")Also applies to: 15-21, 23-25
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/go.yml(1 hunks)internal/commands/webhooks/register.go(2 hunks)tests/e2e/general_test.go(1 hunks)tests/e2e/main_test.go(1 hunks)tests/e2e/messages_test.go(1 hunks)tests/e2e/webhooks_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/e2e/messages_test.go
- tests/e2e/main_test.go
- tests/e2e/webhooks_test.go
- .github/workflows/go.yml
🧰 Additional context used
🧬 Code graph analysis (2)
tests/e2e/general_test.go (1)
cmd/smsgate/smsgate.go (1)
main(22-100)
internal/commands/webhooks/register.go (2)
internal/core/codes/codes.go (1)
ParamsError(5-5)internal/utils/metadata/metadata.go (2)
GetClient(15-17)GetRenderer(23-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: goreleaser
capcom6
force-pushed
the
tests/introduce-e2e-tests
branch
from
471f2ad to
c663e57
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/commands/webhooks/register.go (1)
45-55: Handle URL arg count, case-insensitive scheme, and Go initialism (URL).Currently rejects valid uppercase schemes and doesn't enforce a single positional arg. Also prefer
targetURLper Go initialism.- targetUrl := strings.TrimSpace(c.Args().Get(0)) - if targetUrl == "" { + if c.NArg() != 1 { + return cli.Exit("expected exactly 1 argument: URL", codes.ParamsError) + } + targetURL := strings.TrimSpace(c.Args().Get(0)) + if targetURL == "" { return cli.Exit("URL is empty", codes.ParamsError) } - // accept only absolute http/https URLs - parsed, err := url.Parse(targetUrl) - if err != nil || parsed.Host == "" || (parsed.Scheme != "http" && parsed.Scheme != "https") { + // accept only absolute http/https URLs (scheme is case-insensitive) + parsed, err := url.Parse(targetURL) + if err != nil { + return cli.Exit("invalid URL", codes.ParamsError) + } + scheme := strings.ToLower(parsed.Scheme) + if parsed.Host == "" || (scheme != "http" && scheme != "https") { return cli.Exit("invalid URL", codes.ParamsError) } @@ - URL: targetUrl, + URL: targetURL,Also applies to: 61-61
tests/e2e/messages_test.go (1)
196-257: Nice follow-through: invalid “send” cases now pass explicit bad flags.The
extraArgspath ensures validation is truly exercised for SIM number, priority, base64, and port.
🧹 Nitpick comments (6)
internal/commands/webhooks/register.go (1)
5-5: Optional: use url.ParseRequestURI for stricter validation.
url.ParseRequestURIis often better suited for absolute URLs and avoids some oddities ofurl.Parse.Also applies to: 50-54
tests/e2e/testutils/testutils.go (1)
8-10: Add helper with auto-cleanup to avoid Close() boilerplate.Provide
StartMockServer(t, handler)that registerst.Cleanup(server.Close).import ( "net/http" "net/http/httptest" + "testing" ) func CreateMockServer(handler http.HandlerFunc) *httptest.Server { return httptest.NewServer(handler) } + +// StartMockServer starts a server and auto-closes it via t.Cleanup. +func StartMockServer(t testing.TB, handler http.HandlerFunc) *httptest.Server { + t.Helper() + srv := httptest.NewServer(handler) + t.Cleanup(srv.Close) + return srv +}tests/e2e/messages_test.go (4)
116-141: Avoid panics on JSON type assertions in mock; assert types first.Direct
.(...)on maps can panic; use checked assertions for clearer failures.- // Validate request structure - convert interface{} to []string - phoneNumbers := req["phoneNumbers"].([]any) - expectedPhones := make([]any, len(tt.phones)) - for i, phone := range tt.phones { - expectedPhones[i] = phone - } - assert.Equal(t, expectedPhones, phoneNumbers) - if tt.deviceID != "" { - assert.Equal(t, tt.deviceID, req["deviceId"]) - } - if tt.simNumber > 0 { - assert.Equal(t, float64(tt.simNumber), req["simNumber"]) - } - if tt.priority != 0 { - assert.Equal(t, float64(tt.priority), req["priority"]) - } - if tt.dataMessage { - assert.NotNil(t, req["dataMessage"]) - dataMsg := req["dataMessage"].(map[string]interface{}) - assert.Equal(t, tt.message, dataMsg["data"]) - assert.Equal(t, float64(tt.dataPort), dataMsg["port"]) - } else { - assert.NotNil(t, req["textMessage"]) - textMsg := req["textMessage"].(map[string]interface{}) - assert.Equal(t, tt.message, textMsg["text"]) - } + // Validate request structure with safe type checks + rawPN, ok := req["phoneNumbers"].([]any) + if assert.True(t, ok, "phoneNumbers must be an array") { + expectedPhones := make([]any, len(tt.phones)) + for i, phone := range tt.phones { + expectedPhones[i] = phone + } + assert.Equal(t, expectedPhones, rawPN) + } + if tt.deviceID != "" { + devID, ok := req["deviceId"].(string) + if assert.True(t, ok, "deviceId must be string") { + assert.Equal(t, tt.deviceID, devID) + } + } + if tt.simNumber > 0 { + sn, ok := req["simNumber"].(float64) // JSON numbers -> float64 + if assert.True(t, ok, "simNumber must be number") { + assert.Equal(t, float64(tt.simNumber), sn) + } + } + if tt.priority != 0 { + pr, ok := req["priority"].(float64) + if assert.True(t, ok, "priority must be number") { + assert.Equal(t, float64(tt.priority), pr) + } + } + if tt.dataMessage { + rawDM, ok := req["dataMessage"].(map[string]any) + if assert.True(t, ok, "dataMessage must be object") { + data, ok := rawDM["data"].(string) + if assert.True(t, ok, "data must be string") { + assert.Equal(t, tt.message, data) + } + port, ok := rawDM["port"].(float64) + if assert.True(t, ok, "port must be number") { + assert.Equal(t, float64(tt.dataPort), port) + } + } + } else { + rawTM, ok := req["textMessage"].(map[string]any) + if assert.True(t, ok, "textMessage must be object") { + text, ok := rawTM["text"].(string) + if assert.True(t, ok, "text must be string") { + assert.Equal(t, tt.message, text) + } + } + }
287-290: Loosen brittle stderr matching for flag parsing differences.ur f ave/cli/go-flag wording varies across versions (e.g.,
--sim-numbervs-sim-number). Consider asserting multiple substrings:
- contains "invalid value"
- contains "sim-number"
This reduces false negatives across environments.
176-184: Use CommandContext with a timeout to prevent hung e2e runs.Wrap executions with a context (e.g., 15s) to avoid CI stalls on unexpected hangs.
Example helper (add to a testutils pkg or this file):
func runCLI(t *testing.T, args []string, env map[string]string) (stdout, stderr bytes.Buffer, err error) { t.Helper() ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) defer cancel() cmd := exec.CommandContext(ctx, "./smsgate", args...) cmd.Env = append([]string{}, os.Environ()...) for k, v := range env { cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", k, v)) } cmd.Stdout, cmd.Stderr = &stdout, &stderr err = cmd.Run() if ctx.Err() == context.DeadlineExceeded { t.Fatalf("command timed out: %v", args) } return }Also applies to: 280-288, 362-370, 412-420, 450-458
19-104: Optional: add one JSON-output happy-path to exercise-f json.
expectJSONis declared but unused (always false). Add a test case with-f jsonand parse stdout to validate schema.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/e2e/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
.github/workflows/go.yml(1 hunks)internal/commands/webhooks/register.go(2 hunks)tests/e2e/general_test.go(1 hunks)tests/e2e/go.mod(1 hunks)tests/e2e/main_test.go(1 hunks)tests/e2e/messages_test.go(1 hunks)tests/e2e/testutils/testutils.go(1 hunks)tests/e2e/webhooks_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/e2e/general_test.go
- tests/e2e/go.mod
- .github/workflows/go.yml
- tests/e2e/main_test.go
- tests/e2e/webhooks_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/commands/webhooks/register.go (2)
internal/core/codes/codes.go (1)
ParamsError(5-5)internal/utils/metadata/metadata.go (2)
GetClient(15-17)GetRenderer(23-25)
tests/e2e/messages_test.go (2)
tests/e2e/testutils/testutils.go (1)
CreateMockServer(8-10)cmd/smsgate/smsgate.go (1)
main(22-100)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: goreleaser
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.
Summary by CodeRabbit