Conversation
5aba65c to
783aeb9
Compare
Coverage Report
|
aidandaly24
left a comment
There was a problem hiding this comment.
in general looks good. the JSON.parse thing is just a nit that could be improved in the places that it is used around the update
notgitika
left a comment
There was a problem hiding this comment.
left some comments. overall this is a great start towards end to end tests!!
few things overall we might need to think as follow ups when you head towards a. more complete e2e test suite:
- mechanism for cleaning up resources if tests crash before you run teardown
- adding when are these e2e tests triggered. imo it should be every pre-release.
|
/strands can you review this PR. We don't want to make any changes just want to review and make sure the changes are in line with our code base. Please take a look and leave relevant comments. Don't take actions on the comments. |
0f18d30
Code Review SummaryI've completed a comprehensive review of this PR. Overall, this is solid foundational work for E2E testing with good patterns. Here are the key findings organized by priority: 🔴 High Priority Issues1. Naming Consistency
Recommendation: Standardize on 2. Missing Documentation
3. Removed Test Coverage 🟡 Medium Priority Issues4. JSON Parsing (Lines 44, 82, 106)
5. Test Execution Order (Lines 68-110) 6. Git Config Inconsistency (workflow line 24-26) 🟢 Low Priority / Nits7. Future Work Comment (Line 52) 8. Error Logging Enhancement (Lines 75-77, 99-101) 9. Workflow Improvements
10. Vitest Config (Line 57-58) ✅ What's Done Well
Next StepsThe high-priority items (naming, documentation, coverage validation) should be addressed. The medium-priority items improve code quality and align with existing patterns. Low-priority items are nice-to-haves. This is the right approach for E2E testing - great foundation! 🚀 |
Description
This pr establishes the base for end to end tests.
The first end to end test this adds is create -> deploy -> invoke. I plan to add more coverage (eg. frameworks, memory, in a follow up)
Testing
How have you tested the change?
npm run test:allnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.