[Chore][Fix]: Skip Failing CLI Tests, Rework Failing AI Endpoint Validation test#1258
[Chore][Fix]: Skip Failing CLI Tests, Rework Failing AI Endpoint Validation test#1258
Conversation
The three cli tests that were failing on dev were failing because they required the create mode flag to be set. The hardcode to link made the create paths unreachable. Since we don't have local emulator working, allowing users to pass in opts.mode would be bad practice- they'd be triggering local emulator actions without the local emulator being set up.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRemoved a production-forwarding AI query test and replaced it with a deterministic schema-level invalid-tool test that asserts 400 with "Invalid tool names". Additionally, three CLI init end-to-end tests were marked as skipped with TODO comments. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Pull request overview
Skips three CLI tests that test "create" mode functionality, which is currently unreachable because the init mode was hardcoded to "link" in a previous PR (#1258) while the local emulator isn't ready.
Changes:
- Skip three
it()test cases by changing them toit.skip()with a TODO comment explaining why
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile SummarySkips three CLI e2e tests that test the
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["stack init --mode X"] --> B["runInit()"]
B --> C{"mode = 'link' (hardcoded)"}
C -->|"mode == link/link-config/link-cloud"| D["handleLink()"]
C -->|"mode == create"| E["handleCreate() ❌ UNREACHABLE"]
D --> F{"opts.mode?"}
F -->|"link-config"| G["handleLinkFromConfigFile()"]
F -->|"link-cloud or default"| H["handleLinkFromCloud()"]
E --> I["Writes stack.config.ts"]
style E fill:#ff6b6b,color:#fff
style I fill:#ff6b6b,color:#fff
Last reviewed commit: 0f78042 |
nams1570
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/e2e/tests/backend/endpoints/api/v1/ai-query.test.ts (1)
201-220: Consider consolidating with existing test at lines 116-131.The test at lines 116-131 ("rejects invalid tool names") already covers the same scenario—invalid tool names causing a 400 response. Both tests:
- Are in non-AI describe blocks (run without AI key)
- Use invalid tool names in an otherwise valid request body
- Assert the same response (400 + "Invalid tool names")
If the intent is to have an explicitly "deterministic" test, consider either:
- Moving the existing test (116-131) into this new describe block, or
- Adding a comment explaining how this test differs from the existing one
Also, the comment on line 203 ("for schema validation") is slightly misleading since this test specifically verifies schema validation passes and then business logic validation fails.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/tests/backend/endpoints/api/v1/ai-query.test.ts` around lines 201 - 220, There's a duplicate test scenario: the "AI Query Endpoint - Validation (deterministic)" test that posts an invalid tool name overlaps with the existing "rejects invalid tool names" test; either consolidate by moving the "rejects invalid tool names" test into the "AI Query Endpoint - Validation (deterministic)" describe block (or remove the newer duplicate), or explicitly document how the deterministic test differs; also update the misleading inline comment that says "for schema validation" to state that schema validation passes and business-logic validation (invalid tool names) is expected to fail so the test intent is clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/e2e/tests/backend/endpoints/api/v1/ai-query.test.ts`:
- Around line 201-220: There's a duplicate test scenario: the "AI Query Endpoint
- Validation (deterministic)" test that posts an invalid tool name overlaps with
the existing "rejects invalid tool names" test; either consolidate by moving the
"rejects invalid tool names" test into the "AI Query Endpoint - Validation
(deterministic)" describe block (or remove the newer duplicate), or explicitly
document how the deterministic test differs; also update the misleading inline
comment that says "for schema validation" to state that schema validation passes
and business-logic validation (invalid tool names) is expected to fail so the
test intent is clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b6c0b14b-1e99-432d-a07e-56a0ce5505a3
📒 Files selected for processing (2)
apps/e2e/tests/backend/endpoints/api/v1/ai-query.test.tsapps/e2e/tests/general/cli.test.ts
Previously, this test was flaky because it involved a call to the AI endpoint when it didn't need to. The test's previous comments indicated it was just for schema validation. This test was in a block that was run even when there was no key for the job. Now it's pure validation
nams1570
force-pushed
the
fix-cli-create-tests
branch
from
423f87f to
181efb8
Compare
Context
In a previous PR, we hardcoded the mode to link because local emulator wasn't in a ready state yet. Soon after, we started encountering three failing tests on dev
The three cli tests that were failing on dev were failing because they required the create mode flag to be set. The hardcode to link made the create paths unreachable. Since we don't have local emulator working, allowing users to pass in opts.mode would be bad practice- they'd be triggering local emulator actions without the local emulator being set up.
Also, there was a failing AI endpoint test. The unified AI endpoint tests are set up so that if certain env variables are not present, certain tests aren't run. In practice, if the openrouter key isn't set, the tests that require actually processing a full AI endpoint result without forwarding to prod will be skipped. The failing test was meant to just check schema validation but it performed a full request instead.
Summary of Changes
We just skip the tests for now. They'll only become relevant when "create" is a legitimate workflow, which necessitates the function of local emulator. There is no regression risk because the flow they're testing isn't active yet, and so the only thing we could possibly test is that passing the create mode will invoke a certain function which isn't helpful at this state.
The unified AI endpoint failing test was reworked, another test accomplishes the same schema validation effect. We don't lose coverage by axing the failing test because other AI tests already test valid request bodies (if they weren't valid, they wouldn't get a response).
Summary by CodeRabbit