-
-
Notifications
You must be signed in to change notification settings - Fork 12
chore: reorg test schemas #116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Claude finished @ymc9's task —— View job PR Review: Test Schema ReorganizationReview Tasks:
✅ SummaryThis is a well-executed organizational refactoring that improves the test structure significantly. The changes are primarily mechanical file moves and import path updates with no functional logic changes. 🎯 Positive Aspects1. Improved Organization
2. Consistent Structure
3. Correct Implementation
🔍 Code Quality AssessmentImport Path Changes: ✅ Excellent
Generated Files: ✅ Proper
Schema Generation: ✅ Well-structured
🚨 Potential Issues FoundMinor Issues:
🔒 Security & PerformanceSecurity: ✅ No concerns
Performance: ✅ No impact
📋 Test CoverageCoverage Assessment: ✅ Maintained
💡 Recommendations
🎯 Final AssessmentOverall Rating: ⭐⭐⭐⭐⭐ Excellent This is a textbook example of a good organizational refactoring:
Recommendation: Approve - This PR significantly improves the test structure without any functional risks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR reorganizes the test schema structure by moving schema files into a centralized schemas directory and updates all corresponding import paths.
- Consolidates test schemas under a
schemasdirectory structure - Updates TypeScript schema generation script to process all schema files in the directory
- Improves type safety by adding proper return types for test client functions
Reviewed Changes
Copilot reviewed 35 out of 44 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/sdk/src/ts-schema-generator.ts | Enhanced schema import generation to conditionally include schema object for enums |
| packages/runtime/test/utils.ts | Added proper TypeScript return type for createTestClient function |
| packages/runtime/test/scripts/generate.ts | Modified to process all zmodel files in schemas directory with updated import paths |
| packages/runtime/test/schemas/typing/verify-typing.ts | Updated import path to use new directory structure |
| packages/runtime/test/schemas/typing/schema.ts | Updated import path to use new directory structure |
| packages/runtime/test/schemas/todo/* | Added new generated schema files for todo model |
| packages/runtime/test/schemas/basic/schema.ts | Updated import path to use new directory structure |
| packages/runtime/test/policy/* | Updated import paths and improved type safety |
| packages/runtime/test/plugin/* | Updated import paths to use new schema location |
| packages/runtime/test/client-api/* | Updated all import paths to use new schema location |
| packages/runtime/package.json | Modified build script to include test generation |
Comments suppressed due to low confidence (2)
packages/runtime/test/policy/todo-sample.test.ts:377
- The optional chaining operator suggests that
rcould be null/undefined, but the test doesn't verify this condition. Consider adding a null check assertion before testing the array length to ensure the test fails meaningfully if the query returns null.
expect(r?.lists).toHaveLength(2);
packages/runtime/test/policy/todo-sample.test.ts:383
- Similar to the previous assertion, the optional chaining suggests
r1could be null/undefined. Consider adding a null check assertion before testing the array length to make the test more robust.
expect(r1?.lists).toHaveLength(1);
No description provided.