test(e2e): add messaging providers vitest coverage#5364
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new live Vitest E2E scenario for messaging providers with comprehensive test helpers (env/token parsing, sandbox/host runners, fake Docker APIs, policy rewrites, Slack REST and Discord gateway clients), integrates the scenario as a free-standing CI job, and wires the job into workflow reporting. ChangesMessaging Providers E2E
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
PR Review AdvisorFindings: 0 needs attention, 6 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27438223405
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27438710530
|
Collaborator
Author
|
Addressing the PR Review Advisor Slack-policy worth-check note: the runtime base-policy premerge is intentional migration parity with |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@test/e2e-scenario/live/messaging-providers-helpers.ts`:
- Around line 767-773: encodeClientText currently always emits the 7-bit
payload-length form which breaks for the IDENTIFY JSON (lines ~836-844) that
exceeds 125 bytes; update encodeClientText to encode extended WebSocket payload
lengths properly: if payload length <=125 use 0x80|len, if <=65535 use 0x80|126
followed by a 2-byte big-endian length, otherwise use 0x80|127 followed by an
8-byte big-endian length, then append the 4-byte mask and masked payload as
before so the IDENTIFY frame is valid for strict gateways.
In `@test/e2e-scenario/live/messaging-providers.test.ts`:
- Around line 603-653: The inline Node probe passed to sandboxOutput should
never terminate with a non-zero exit because sandboxOutput throws on non-zero
exit codes; update the script inside the discordReach call so it always exits 0
(e.g., replace process.exit(failed ? 1 : 0) with process.exit(0) or remove
explicit exiting and let the script naturally end) and keep the existing
logging/failed flag behavior so the subsequent checks (discordReach, check,
skipNote) still run; locate the Node snippet used to build discordReach in the
test and modify the final done() completion path accordingly.
- Around line 174-176: The check currently passes if SANDBOX_NAME appears
anywhere and any sandbox is Ready; change the condition so it searches
sandboxList.stdout for a single line that contains both the SANDBOX_NAME and the
word "Ready" (i.e., match them on the same row). In the check that uses
sandboxList.stdout and SANDBOX_NAME, replace the boolean expression with a
multiline regex that finds a line containing the escaped SANDBOX_NAME and
"Ready" together (ensure you escape any regex metacharacters in SANDBOX_NAME
before building the regex) so the test only succeeds when the target sandbox row
is Ready.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4dcbd397-c2f0-47d3-85c5-a6c237696236
📒 Files selected for processing (5)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/live/messaging-providers-helpers.tstest/e2e-scenario/live/messaging-providers.test.tstest/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tstools/e2e-scenarios/free-standing-jobs.env
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27444259885
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27445134225
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cv
changed the title
79 tasks
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
3 participants
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
Adds focused live Vitest coverage for the highest-value messaging-provider paths from the legacy
test/e2e/test-messaging-providers.shscenario and wires that focused scenario into e2e workflow dispatch. This PR does not retire the entire legacy shell contract: Telegram inbound replies, Slack mention/reply feedback, revoked Slack token pre-validation, and no-real-secret plugin-send fallback paths remain in the shell suite until their own scoped migrations.The new coverage keeps fake-token defaults,
_REALsecret overrides, redaction assertions, optional live sends, QR-only WhatsApp behavior, and evidence-rich skips for NVIDIA endpoint validation outages before provider assertions can run.Related Issue
Refs #5098
Changes
test/e2e-scenario/live/messaging-providers.test.tscovering provider registration, redaction checks, fake API rewrite probes, optional live sends, and best-effort cleanup.test/e2e-scenario/live/messaging-providers-helpers.tsfor local helper plumbing so the focused*.test.tsstays under the default test-size guardrail.messaging-providers-vitestto.github/workflows/e2e-vitest-scenarios.yamlwith the same optional secret contract as the shell scenario.Type of Change
Verification
Targeted checks run locally:
npm ci --ignore-scriptsnpm run build:clicd nemoclaw && npm ci --ignore-scripts && npm run buildnpx @biomejs/biome check test/e2e-scenario/live/messaging-providers.test.ts test/e2e-scenario/live/messaging-providers-helpers.ts test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts .github/workflows/e2e-vitest-scenarios.yaml tools/e2e-scenarios/free-standing-jobs.env ci/test-file-size-budget.jsonnpm run test-size:checknpm run source-shape:checknpx vitest run --project e2e-vitest-support test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tsNEMOCLAW_RUN_E2E_SCENARIOS=1 npx vitest run --project e2e-scenarios-live test/e2e-scenario/live/messaging-providers.test.ts(skips locally withoutNVIDIA_API_KEY)npm run typecheck:cli -- --pretty falsenpx vitest run --project cli test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tsgit diff --checknpx prek run --files .github/workflows/e2e-vitest-scenarios.yaml ci/test-file-size-budget.json test/e2e-scenario/live/messaging-providers.test.ts test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts tools/e2e-scenarios/free-standing-jobs.envGitHub verification:
messaging-providers-vitest: https://github.com/NVIDIA/NemoClaw/actions/runs/27445134225Local note: the full
test-clihook was attempted duringgit commitandgit push, but hit unrelated 5s local timeouts in CLI help / Brave web-search validation tests. The branch was committed and pushed with onlytest-cliskipped; pre-push TypeScript and all migration-focused checks passed.npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
Tests
Chores