tests(api) add script to test the claim token api #320
Conversation
akaladarshi
requested review from
Copilot,
elmattic and
sudo-shashank
and removed request for
Copilot
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a matrix-driven GitHub Actions E2E workflow that launches isolated Durable Object servers per matrix entry, adds k6 API and CORS E2E tests with configs, makes E2E BASE_URL configurable via env, and introduces documentation for API and CORS E2E testing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor GH as GitHub Actions
participant Runner as Job Runner
participant Cache as Cache Restore
participant Tools as Tooling Install
participant Wrangler as wrangler dev (DO)
participant Readiness as Port/HTTP Checks
participant K6 as k6 Runner
participant API as /api/claim_token
GH->>Runner: start matrix entry (Browser / API / CORS)
Runner->>Cache: restore cargo/npm caches
Runner->>Tools: install wasm-opt & toolchain
Runner->>Wrangler: wrangler dev --port matrix.port --state-dir matrix.state_dir
Wrangler->>Readiness: bind/listen on matrix.port
Readiness-->>Runner: port + HTTP ready
Runner->>K6: run matrix.script (API_URL=http://127.0.0.1:matrix.port)
K6->>API: OPTIONS/GET /api/claim_token?... (connectivity / CORS / validation)
alt success (200)
API-->>K6: 200 + tx hash
else limited / mainnet / error
API-->>K6: 429 / 418 / 4xx/5xx (with CORS headers)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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). (6)
🔇 Additional comments (7)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #320 +/- ##
=======================================
Coverage 38.75% 38.75%
=======================================
Files 40 40
Lines 2578 2578
=======================================
Hits 999 999
Misses 1579 1579 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
e2e/test_claim_token_api.js (2)
12-21: Consider adding threshold for error rate metric.The
errorRatemetric is tracked but has no threshold defined. Consider adding a threshold like'errors': ['rate<0.1']to fail the test if error rate exceeds 10%.Apply this diff:
export const options = { vus: 1, iterations: 1, thresholds: { 'http_req_duration': ['p(95)<5000'], + 'errors': ['rate<0.1'], }, };
23-30: Enhance Filecoin transaction hash validation.The Filecoin validation only checks length (46-70 characters) but doesn't verify the format. Filecoin CIDs typically start with
bafyfor base32-encoded multihash. Consider adding a prefix check for stronger validation.Apply this diff:
function validateTransactionHash(txHash, expectedFormat) { if (expectedFormat === 'ethereum') { return txHash.startsWith('0x') && txHash.length === 66; } else if (expectedFormat === 'filecoin') { - return txHash.length >= 46 && txHash.length <= 70; + return txHash.startsWith('bafy') && txHash.length >= 46 && txHash.length <= 70; } return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/e2e.yml(1 hunks)e2e/test_claim_token_api.js(1 hunks)e2e/test_claim_token_api_config.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
e2e/test_claim_token_api.js (1)
e2e/test_claim_token_api_config.js (10)
API_CONFIG(2-10)API_CONFIG(2-10)FaucetTypes(34-39)FaucetTypes(34-39)TEST_ADDRESSES(12-32)TEST_ADDRESSES(12-32)STATUS_CODES(41-47)STATUS_CODES(41-47)TEST_SCENARIOS(50-194)TEST_SCENARIOS(50-194)
⏰ 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). (4)
- GitHub Check: codedov
- GitHub Check: lint
- GitHub Check: e2e
- GitHub Check: deploy
🔇 Additional comments (9)
.github/workflows/e2e.yml (1)
81-84: LGTM!The new workflow step correctly integrates the claim_token API tests into the CI pipeline using the same pattern as the existing k6 step.
e2e/test_claim_token_api_config.js (4)
34-47: LGTM!Faucet types and status codes are well-defined and align with the API requirements from issue #319.
99-193: Excellent rate-limiting test coverage.The test scenarios comprehensively cover rate limiting across different faucet types, address formats, and address equivalence (ETH ↔ t410). This aligns well with the requirements in issue #319.
54-74: Tests are correct: missing parameters yield 500 TheGetUrlextractor fails on absent query fields and surfaces them asINTERNAL_SERVER_ERROR, so keeping the tests’ expectations at 500 is accurate.
15-16: ETH_FORMAT_ADDRESS and T410_ADDRESS correctly correspond. No change required.e2e/test_claim_token_api.js (4)
32-52: LGTM!The request helper properly handles optional parameters, applies URL encoding, and configures timeouts and tags appropriately.
54-85: LGTM!Input validation tests comprehensively cover invalid addresses and malformed requests, addressing the requirements from issue #319 for testing
parse_and_validate_addressbehavior.
87-114: LGTM!Rate limiting tests thoroughly validate the 429 response requirement from issue #319, including transaction hash format verification for successful claims and proper handling of address equivalence.
116-119: LGTM!Test orchestration is clean and runs validation tests before rate limiting tests in a logical sequence.
akaladarshi
force-pushed
the
akaladarshi/claim-token-api-tests
branch
from
08faae4 to
a98ce34
Compare
akaladarshi
force-pushed
the
akaladarshi/claim-token-api-tests
branch
from
a98ce34 to
68b4178
Compare
akaladarshi
force-pushed
the
akaladarshi/claim-token-api-tests
branch
2 times, most recently
from
7604d27 to
54d2c5f
Compare
akaladarshi
force-pushed
the
akaladarshi/claim-token-api-tests
branch
from
54d2c5f to
bb16e24
Compare
akaladarshi
force-pushed
the
akaladarshi/claim-token-api-tests
branch
from
bad61b9 to
2fb3d4c
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive end-to-end testing for the claim token API using k6. The testing suite validates API behavior including input validation, rate limiting mechanisms, and response format verification across different faucet types and address formats.
- Implements k6 test script with configurable test scenarios for claim token API validation
- Adds comprehensive test cases covering invalid inputs, rate limiting behavior, and transaction hash validation
- Integrates new test suite into existing CI/E2E workflow for automated verification
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| e2e/test_claim_token_api_config.js | Configuration file defining test addresses, faucet types, status codes, and test scenarios |
| e2e/test_claim_token_api.js | Main test script implementing validation logic and test execution |
| .github/workflows/e2e.yml | CI workflow update to include the new claim token API test suite |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e/test_claim_token_api.js (1)
95-108: AssertRetry-Afteron rate-limited responses.Issue #319 asks us to verify the retry-after messaging when rate limiting kicks in. We only assert the 429 status today, so the header/message could regress unnoticed. Please add a
checkthat inspectsresponse.headers['Retry-After'](or the equivalent body field if that’s how the API surfaces it).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/e2e.yml(1 hunks)e2e/test_claim_token_api.js(1 hunks)e2e/test_claim_token_api_config.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
e2e/test_claim_token_api.js (1)
e2e/test_claim_token_api_config.js (10)
API_CONFIG(2-10)API_CONFIG(2-10)FaucetTypes(34-39)FaucetTypes(34-39)TEST_ADDRESSES(12-32)TEST_ADDRESSES(12-32)STATUS_CODES(41-47)STATUS_CODES(41-47)TEST_SCENARIOS(50-165)TEST_SCENARIOS(50-165)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
e2e/test_claim_token_api.js (3)
22-28: Redundant quote stripping suggests uncertainty about response format.The logic strips quotes twice (lines 22-24, then 26-28), which would only be necessary for triple-quoted strings like
"""hash""". This redundancy suggests uncertainty about the actual response format.Investigate the actual format returned by the API and simplify to a single stripping operation:
function validateTransactionHash(txHash) { - // Remove outer quotes if present if (txHash.startsWith('"') && txHash.endsWith('"')) { txHash = txHash.slice(1, -1); } - // Remove inner quotes if present - if (txHash.startsWith('"') && txHash.endsWith('"')) { - txHash = txHash.slice(1, -1); - } - - // Both CalibnetFIL and CalibnetUSDFC now return Ethereum format: 0x + 64 hex chars = 66 total return txHash.startsWith('0x') && txHash.length === 66; }Alternatively, if the API genuinely returns nested quotes, add a comment explaining why.
66-71: Consider extracting check name construction.The inline check name construction on line 67 works but reduces readability. Consider extracting it:
+ const checkName = `${faucetType} - Invalid address "${invalidAddress}" properly rejected (400 and error)`; check(response, { - [checkName]: (r) => + [checkName]: (r) => r.status === STATUS_CODES.BAD_REQUEST && r.body && r.body.toLowerCase().includes("invalid") });This makes the check assertion more readable while maintaining the same functionality.
120-121: Clarify the 5-second buffer in cooldown wait.The wait uses
FAUCET_COOLDOWN_BUFFER_SECONDS(65s) but doesn't explain the 5-second buffer beyond the 60-second cooldown. Consider improving the comment:- console.log(`⏰ Waiting ${API_CONFIG.FAUCET_COOLDOWN_BUFFER_SECONDS} seconds to ensure previous global faucet cooldowns have expired...`); + console.log(`⏰ Waiting ${API_CONFIG.FAUCET_COOLDOWN_BUFFER_SECONDS} seconds (60s cooldown + 5s buffer) to ensure previous faucet cooldowns have expired...`);This makes the buffer explicit in the log message, helping future maintainers understand why it's not exactly 60 seconds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e/test_claim_token_api.js(1 hunks)e2e/test_claim_token_api_config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/test_claim_token_api_config.js
🧰 Additional context used
🧬 Code graph analysis (1)
e2e/test_claim_token_api.js (1)
e2e/test_claim_token_api_config.js (10)
API_CONFIG(2-12)API_CONFIG(2-12)FaucetTypes(36-41)FaucetTypes(36-41)TEST_ADDRESSES(14-34)TEST_ADDRESSES(14-34)STATUS_CODES(43-49)STATUS_CODES(43-49)TEST_SCENARIOS(52-167)TEST_SCENARIOS(52-167)
⏰ 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). (4)
- GitHub Check: deploy
- GitHub Check: lint
- GitHub Check: codedov
- GitHub Check: e2e
🔇 Additional comments (3)
e2e/test_claim_token_api.js (3)
1-18: LGTM! Clean test setup.The k6 imports and options configuration are well-structured. Single VU and iteration ensure deterministic sequential execution, which is appropriate for testing rate-limiting behavior that depends on order.
34-53: LGTM! Proper URL construction and request handling.The function correctly handles optional parameters, applies URL encoding, and configures appropriate timeout and tags for the HTTP request.
90-116: LGTM! Thorough rate-limiting test coverage.The test suite properly validates both successful claims and rate-limited rejections, validates transaction hashes on success, and includes helpful debug logging. The 0.5-second sleep between requests is appropriate pacing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
e2e/test_claim_token_api.js (2)
20-32: Clarify or fix duplicate quote-removal logic.Lines 22-23 and 26-27 contain identical logic for removing quotes. If this is intentional to handle doubly-nested quotes, add a comment explaining why. Otherwise, this appears to be a copy-paste error. Typically, you'd only need to remove quotes once.
Apply this diff to simplify the function:
function validateTransactionHash(txHash) { // Remove outer quotes if present if (txHash.startsWith('"') && txHash.endsWith('"')) { txHash = txHash.slice(1, -1); } - // Remove inner quotes if present - if (txHash.startsWith('"') && txHash.endsWith('"')) { - txHash = txHash.slice(1, -1); - } // Both CalibnetFIL and CalibnetUSDFC now return Ethereum format: 0x + 64 hex chars = 66 total return txHash.startsWith('0x') && txHash.length === 66; }
162-162: Add semicolon for consistency.Line 162 is missing a semicolon after
testWalletCap(). While JavaScript doesn't strictly require it, adding one would maintain consistency with the rest of the codebase.testRateLimiting(); - testWalletCap() + testWalletCap(); console.log('\n✅ All tests passed successfully!');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e/test_claim_token_api.js(1 hunks)e2e/test_claim_token_api_config.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
e2e/test_claim_token_api.js (1)
e2e/test_claim_token_api_config.js (10)
API_CONFIG(2-12)API_CONFIG(2-12)FaucetTypes(36-41)FaucetTypes(36-41)TEST_ADDRESSES(14-34)TEST_ADDRESSES(14-34)STATUS_CODES(43-49)STATUS_CODES(43-49)TEST_SCENARIOS(52-305)TEST_SCENARIOS(52-305)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
e2e/test_claim_token_api.js (1)
20-25: Transaction hash validation still accepts non-hex characters.This issue was previously flagged: the current check only validates prefix and length, allowing malformed hashes like
0xZZZZ...to pass validation. The suggested regex-based approach ensures all 64 characters after0xare valid hexadecimal.Apply the previously suggested fix:
- return txHash.startsWith('0x') && txHash.length === 66; + return /^0x[0-9a-fA-F]{64}$/.test(txHash);
🧹 Nitpick comments (1)
e2e/test_claim_token_api.js (1)
148-156: Consider more robust retry-time parsing.The regex
/(\d+)/captures the first digit sequence in the response body, which could break if the API response format evolves to include other numbers (e.g., "You've claimed 2 times. Retry in 3600 seconds"). Either use a more specific pattern that anchors to expected text (e.g.,/retry.*?(\d+)\s*seconds?/i) or document the assumed response format to help future maintainers catch regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e/test_claim_token_api.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
e2e/test_claim_token_api.js (1)
e2e/test_claim_token_api_config.js (10)
STATUS_CODES(72-78)STATUS_CODES(72-78)API_CONFIG(2-13)API_CONFIG(2-13)FaucetTypes(65-70)FaucetTypes(65-70)TEST_ADDRESSES(15-63)TEST_ADDRESSES(15-63)TEST_SCENARIOS(81-334)TEST_SCENARIOS(81-334)
⏰ 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). (4)
- GitHub Check: codedov
- GitHub Check: deploy
- GitHub Check: E2E Browser Tests
- GitHub Check: E2E API Tests
🔇 Additional comments (2)
e2e/test_claim_token_api.js (2)
27-69: Well-structured helper with clear fail-fast logic.The refactored
runTestScenariosconsolidates common test execution patterns effectively:
- Configurable waiting and sleep intervals
- Primary status check before additional validations prevents cascading false positives
- Extensible via
additionalCheckscallback for test-specific assertions- Clear debug logging when checks fail
This addresses the previous feedback to extract shared logic between rate-limiting and wallet-cap tests.
164-188: Robust connectivity check with appropriate retry logic.The server health check provides a good safety net before test execution:
- Three retry attempts with clear logging at each step
- 5-second intervals give the server time to become ready
- Boolean return allows the main test runner to abort gracefully
- Explicit timeout prevents hanging on unresponsive servers
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/e2e.yml (1)
3-5: Fix boolean expression quoting in concurrency.cancel-in-progress expects a boolean/expression, not a quoted string. Unquote to ensure proper behavior.
Apply this diff:
concurrency: group: "${{ github.workflow }}-${{ github.ref }}" - cancel-in-progress: "${{ github.ref != 'refs/heads/main' }}" + cancel-in-progress: ${{ github.ref != 'refs/heads/main' }}
🧹 Nitpick comments (10)
e2e/test_claim_token_api_config.js (2)
212-213: DRY up cooldown waits.Use the existing FAUCET_COOLDOWN_BUFFER_SECONDS constant instead of hardcoded 65s to keep tests in sync if the buffer changes.
Apply this diff:
- waitBefore: 65, // Wait for cooldown from the main rate-limit tests to expire + waitBefore: API_CONFIG.FAUCET_COOLDOWN_BUFFER_SECONDS, // Wait for cooldown from the main rate-limit tests to expire @@ - waitBefore: 65, // Wait for cooldown from its own 2nd transaction + waitBefore: API_CONFIG.FAUCET_COOLDOWN_BUFFER_SECONDS, // Wait for cooldown from its own 2nd transaction @@ - waitBefore: 65, // Wait for cooldown from the previous test group + waitBefore: API_CONFIG.FAUCET_COOLDOWN_BUFFER_SECONDS, // Wait for cooldown from the previous test group @@ - waitBefore: 65, // Wait for its own cooldown + waitBefore: API_CONFIG.FAUCET_COOLDOWN_BUFFER_SECONDS, // Wait for its own cooldown @@ - waitBefore: 65, // Wait for its own cooldown + waitBefore: API_CONFIG.FAUCET_COOLDOWN_BUFFER_SECONDS, // Wait for its own cooldown @@ - waitBefore: 65, // Wait for cooldown from the previous test group to expire + waitBefore: API_CONFIG.FAUCET_COOLDOWN_BUFFER_SECONDS, // Wait for cooldown from the previous test group to expire @@ - waitBefore: 65, // Wait for cooldown from its own 2nd transaction + waitBefore: API_CONFIG.FAUCET_COOLDOWN_BUFFER_SECONDS, // Wait for cooldown from its own 2nd transactionAlso applies to: 220-221, 230-231, 238-239, 246-247, 280-281, 288-289
80-142: Make error matching resilient.Error strings (“missing”, “unknown variant”, “invalid address”, “teapot”) can vary with framework/serde versions. Ensure the assertion is case-insensitive and trims noise (or assert status only in k6 and leave exact error matching to Rust unit/integration tests).
.github/workflows/e2e.yml (2)
112-116: Remove nc dependency; rely on curl-only readiness checks.Ubuntu runners may lack nc; using curl-only simplifies and avoids a missing-binary failure.
Apply this diff:
- echo "waiting for server on port ${{ matrix.port }}" - timeout 120 sh -c 'until nc -z $0 $1; do sleep 1; done' 127.0.0.1 ${{ matrix.port }} - echo "TCP port check passed, now waiting for HTTP server to be ready..." - timeout 120 sh -c 'until curl -s --max-time 30 http://127.0.0.1:${{ matrix.port }} > /dev/null; do echo "Waiting for HTTP server (first request may take 60-90s)..."; sleep 5; done' + echo "Waiting for HTTP server to be ready..." + timeout 180 sh -c 'until curl -fsS --max-time 10 http://127.0.0.1:${{ matrix.port }} >/dev/null; do echo "Waiting for HTTP server (first request may take 60-90s)..."; sleep 5; done' echo "Server is ready!"
90-96: Avoid echoing secrets directly in logs.GitHub masks values, but it’s safer to prevent any accidental leakage. Prefer a here-doc to avoid repeating secrets in shell history/log lines.
Apply this diff:
- echo "SECRET_WALLET=${{ secrets.TEST_CALIBNET_PRIVATE_KEY_HEX }}" > .dev.vars - echo "SECRET_MAINNET_WALLET=${{ secrets.TEST_MAINNET_PRIVATE_KEY_HEX }}" >> .dev.vars - echo "SECRET_CALIBNET_USDFC_WALLET=${{ secrets.TEST_CALIBNET_USDFC_PRIVATE_KEY_HEX }}" >> .dev.vars + cat > .dev.vars <<'EOF' +SECRET_WALLET=${{ secrets.TEST_CALIBNET_PRIVATE_KEY_HEX }} +SECRET_MAINNET_WALLET=${{ secrets.TEST_MAINNET_PRIVATE_KEY_HEX }} +SECRET_CALIBNET_USDFC_WALLET=${{ secrets.TEST_CALIBNET_USDFC_PRIVATE_KEY_HEX }} +EOFe2e/test_cors.js (2)
100-111: Also assert Retry-After is exposed on 429 responses.Frontends often need Retry-After for backoff. Verify both the header’s presence and that CORS exposes it.
Apply this diff:
check(securityResponse, { 'Security: Has Access-Control-Allow-Origin': (r) => r.headers['Access-Control-Allow-Origin'] !== undefined, 'Security: Access-Control-Allow-Credentials is not set (safer for public APIs)': (r) => r.headers['Access-Control-Allow-Credentials'] === undefined || r.headers['Access-Control-Allow-Credentials'] === 'false', }); + // Test 6.1: If rate-limited, ensure Retry-After is present and exposed via CORS + const rateLimitProbe1 = http.get(url, { headers: { 'Origin': 'https://external-example.com' } }); + const rateLimitProbe2 = http.get(url, { headers: { 'Origin': 'https://external-example.com' } }); + check(rateLimitProbe2, { + '429: Has Retry-After header when rate limited (if applicable)': (r) => + r.status !== 429 || r.headers['Retry-After'] !== undefined, + '429: Access-Control-Expose-Headers includes Retry-After (or is *)': (r) => { + if (r.status !== 429) return true; + const expose = (r.headers['Access-Control-Expose-Headers'] || '').toLowerCase(); + return expose === '*' || expose.includes('retry-after'); + }, + });
73-76: Consider relaxing same-origin CORS header expectation.Some servers omit CORS headers for same-origin. If portability matters, allow header absence for same-origin. Optional.
docs/cors_testing_guide.md (4)
51-57: Sync port with the workflow (uses 8787).Either update docs to 8787 or set the CORS matrix to 8788. Keep them consistent to avoid confusion.
Apply this diff:
- port: 8788 # Separate port from Browser/API tests + port: 8787 # Same port as CI workflow configuration
147-147: Use a heading instead of emphasis (markdownlint MD036).Change bold emphasis to a proper heading.
Apply this diff:
-**Total: 23 checks covering full CORS compliance** +### Total: 23 checks covering full CORS compliance
163-189: Add fenced code languages (markdownlint MD040).Mark blocks as text for expected output to improve readability and linting.
Apply this diff:
-``` +```text checks_succeeded...: 100.00% 23 out of 23 @@ ✓ Error Response: CORS headers present even on errors--- `135-145`: **Document exposing Retry-After for browsers.** Mention Access-Control-Expose-Headers so UIs can read Retry-After on 429. Apply this diff: ```diff ### 5. Security & Edge Cases (3 checks) @@ -✓ CORS headers present even on 500/400 errors (critical for browser error handling) +✓ CORS headers present even on 500/400 errors (critical for browser error handling) +✓ On 429, Retry-After is present and exposed via `Access-Control-Expose-Headers` (or wildcard `*`)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/e2e.yml(4 hunks)docs/cors_testing_guide.md(1 hunks)e2e/test_claim_token_api_config.js(1 hunks)e2e/test_cors.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
e2e/test_cors.js (1)
e2e/test_claim_token_api_config.js (4)
FaucetTypes(65-70)FaucetTypes(65-70)TEST_ADDRESSES(15-63)TEST_ADDRESSES(15-63)
🪛 markdownlint-cli2 (0.18.1)
docs/cors_testing_guide.md
147-147: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
163-163: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (5)
- GitHub Check: E2E Claim Token API CORS Tests
- GitHub Check: E2E Browser Tests
- GitHub Check: E2E API Tests
- GitHub Check: deploy
- GitHub Check: codedov
🔇 Additional comments (2)
e2e/test_cors.js (1)
32-49: Preflight coverage looks good.Validating status, A-C-A-Origin, methods, and requested headers is sufficient here.
e2e/test_claim_token_api_config.js (1)
144-205: Now I need to check the e2e test file to understand how it currently handles the 60+ second waits:I need to examine the actual e2e test file to understand how the 60+ second waits are being used:Now I have comprehensive information. The review comment is suggesting that the e2e tests use environment variables to override rate-limit settings. Based on my verification:
Key Findings:
- The Rust backend rate-limit constants (MAINNET_COOLDOWN_SECONDS=600, CALIBNET_COOLDOWN_SECONDS=60) are hardcoded - NOT environment-configurable
- Only
RATE_LIMITER_DISABLEDenv var exists, which disables the entire rate limiter (not suitable for testing the behavior itself)- The codebase would need refactoring to make cooldown and wallet-cap configurable via environment variables
- The review suggests a solution that is NOT currently feasible without code changes to the Rust backend
The review comment is requesting a feature that doesn't exist in the codebase. The suggestion to "override cooldown and wallet-cap via test-only env" requires implementation work that hasn't been done.
Shorten CI duration by configuring test-only rate-limit settings.
The 60+ second waits will inflate runtime significantly. To implement this suggestion, the backend rate-limit constants (MAINNET_COOLDOWN_SECONDS, CALIBNET_COOLDOWN_SECONDS, wallet-cap multipliers in
src/faucet/constants.rs) must be made configurable via environment variables with fallback defaults. Currently, onlyRATE_LIMITER_DISABLEDexists (which disables all rate limiting entirely).Suggested approach:
- Refactor
FaucetInfo::rate_limit_seconds()andFaucetInfo::wallet_cap()methods to read from environment variables (e.g.,TEST_COOLDOWN_SECONDS,TEST_WALLET_CAP_MULTIPLIER) before falling back to hardcoded constants- Update
wrangler.tomlto allow setting these vars for test environment- Then e2e tests can set short test intervals (2–3s cooldown, cap=2) while keeping the same test scenario logic intact
akaladarshi
force-pushed
the
akaladarshi/claim-token-api-tests
branch
from
e83b66a to
9a346fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
e2e/test_claim_token_api_config.js (2)
12-12: Good buffer choice; consider documenting the rationale.The 65-second buffer (5 seconds over the 60-second cooldown) is a sensible safety margin for timing-sensitive tests, but the reason for the extra 5 seconds isn't immediately obvious. Consider adding an inline comment explaining that this provides a buffer to account for timing variations in test execution.
- FAUCET_COOLDOWN_BUFFER_SECONDS: 65, + FAUCET_COOLDOWN_BUFFER_SECONDS: 65, // 60s cooldown + 5s buffer for timing variations
300-334: Track the commented ID wallet tests.The commented-out test cases for CalibnetUSDFC ID wallet are reasonable given the uncertainty about token recovery, but this represents a gap in test coverage.
Would you like me to create a tracking issue for implementing these tests once the token recovery mechanism is clarified? This would ensure comprehensive wallet cap testing across all address formats.
docs/cors_testing_guide.md (2)
148-148: Use heading instead of emphasis.For better document structure, convert the bold text to a proper heading.
-**Total: 22 checks covering full CORS compliance** +### Total: 22 checks covering full CORS compliance
164-189: Add language identifier to code block.The fenced code block should specify a language for proper syntax highlighting.
-``` +```text checks_succeeded...: 100.00% 22 out of 22
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/e2e.yml(4 hunks)docs/cors_testing_guide.md(1 hunks)e2e/test_claim_token_api_config.js(1 hunks)e2e/test_cors.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/test_cors.js
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
docs/cors_testing_guide.md
148-148: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
164-164: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (5)
- GitHub Check: deploy
- GitHub Check: codedov
- GitHub Check: E2E API Tests
- GitHub Check: E2E Claim Token API CORS Tests
- GitHub Check: E2E Browser Tests
🔇 Additional comments (5)
e2e/test_claim_token_api_config.js (1)
15-63: Excellent address coverage!The test addresses cover all major formats (F1, T1, T410, ETH, T0, ID) and the invalid address list includes comprehensive edge cases (malformed ETH addresses, invalid checksums, unsupported protocols). This addresses the previous review feedback thoroughly.
.github/workflows/e2e.yml (3)
27-46: Matrix strategy looks solid!The fail-fast: false configuration ensures all test suites run to completion even if one fails, providing comprehensive test results. Each matrix entry correctly uses the same port (8787) since they run on separate GitHub-hosted runners, avoiding conflicts.
110-111: Good state isolation strategy.Using per-matrix and per-run state directories (
--persist-to .wrangler-state-${{ matrix.state_dir }}-${{ github.run_id }}) provides excellent isolation between parallel test runs. In CI, these are ephemeral since each workflow run gets a fresh checkout. For local development, developers should be aware that these directories will accumulate and may need periodic cleanup.
112-116: Excellent readiness checks.The two-phase readiness validation (TCP port check followed by HTTP availability check) with clear progress messages and appropriate timeouts (120s total) is a robust approach for handling the 60-90s first-request warm-up time.
docs/cors_testing_guide.md (1)
1-258: Comprehensive and well-structured guide.The CORS testing documentation provides excellent coverage of terminology, configuration, test flow, and troubleshooting. The examples are practical and the explanations are clear. Once the port inconsistency is corrected, this will be a valuable resource for understanding and running the CORS tests.
akaladarshi
force-pushed
the
akaladarshi/claim-token-api-tests
branch
from
01bf0d8 to
836fb69
Compare
Summary of changes
Changes introduced in this pull request:
claim_tokenAPIReference issue to close (if applicable)
Closes #319
Other information and links
Change checklist
adheres to the team's
documentation standards,
(if possible),
should be reflected in this document.
Summary by CodeRabbit