🏗️ Add release infrastructure for Ember SDK #167
Conversation
Code ReviewGreat work on adding complete release infrastructure for the Ember SDK! The implementation is well-structured and consistent with other client SDKs. Here's my detailed review: ✅ Strengths1. Excellent ConsistencyThe release workflow closely matches existing client patterns (vitest, storybook, etc.), which makes it maintainable and reduces cognitive load for the team. 2. Comprehensive CI CoverageThe CI test job (
3. Prerelease SupportThe 4. Proper IntegrationThe 🐛 Issues & Concerns1. Missing Integration Test Step in Release WorkflowSeverity: Medium The release workflow runs unit tests and linter but doesn't run integration tests before publishing. This is inconsistent with the CI workflow which runs Location: Impact: You could publish a release that passes unit tests but fails integration tests. Recommendation: Add this step after line 114: - name: Run integration tests
working-directory: ./clients/ember
run: npm run test:integration2. No E2E Visual Test Step in Release WorkflowSeverity: Low-Medium The release workflow doesn't run the E2E visual tests that are part of CI ( Location: Recommendation: Consider whether E2E tests should run before release. If not, document why in comments. 3. Lint Errors Fix Without ContextSeverity: Low The PR fixes lint errors (unused params with Location: Question: Were these intentional unused params or oversight? The fix is correct, but context helps. 🔍 Security & Performance4. Git Auth ReconfigurationSeverity: Info Line 150-154 reconfigures git auth by manually setting the HTTP extraheader. This pattern exists in other release workflows too. Question: Is this necessary? The checkout action already uses a token. If git config is being reset by npm version or other commands, consider investigating why rather than re-authenticating. Potential Issue: Hardcoded base64 encoding of credentials in logs (though GitHub should mask secrets). 5. Playwright Browser CachingSeverity: Info ✅ Good use of caching! The cache key properly includes the Playwright version and only installs Suggestion: Consider adding a timeout or fallback if 📝 Code Quality6. CHANGELOG.md FormatSeverity: Low The changelog has an "Architecture" section under the first release, which is unusual for a changelog. Typically this would go in README or docs. Location: Recommendation: Keep changelogs focused on what changed rather than how it works. Move architecture explanation to README. 7. Changelog DateSeverity: Info The changelog is dated 2026-01-04, which matches the current system date. Just flagging in case the release happens on a different day. 🧪 Test Coverage8. Missing Test Plan VerificationSeverity: Info The PR test plan has unchecked boxes:
Reminder: Verify these before merging, especially the token configuration. 💡 Suggestions9. Version Bump Logic InconsistencySeverity: Low When creating a prerelease, the workflow uses npm version with a 'pre' prefix. This means:
Question: Is this the intended behavior? For subsequent betas, you might want Reference: https://docs.npmjs.com/cli/v9/commands/npm-version 10. Hardcoded Node VersionThe release workflow hardcodes Node 22, while CI tests both 22 and 24. Consider using a matrix or testing with the same version that will run the release. 🎯 SummaryApprove with minor changes recommended. The infrastructure is solid and well-implemented. The main concern is missing integration tests in the release workflow, which should be addressed before publishing to npm. The other issues are minor and can be addressed post-merge or left as-is with team discussion. Action Items
Great job on comprehensive coverage and matching patterns! 🚀 |
Robdel12
force-pushed
the
rd/ember-sdk-testing
branch
from
525701f to
798ba33
Compare
- Add release-ember-client.yml workflow with prerelease support - Add CI test job with Playwright browser caching (blacksmith runner) - Add full E2E visual tests running through vizzly run (needs VIZZLY_EMBER_CLIENT_TOKEN) - Only run visual E2E tests on Node 22 to avoid duplicate uploads (fixes Vitest too) - Add 8-minute timeout to all CI jobs to prevent hangs - Add CHANGELOG.md for initial beta release - Add LICENSE symlink - Update version to 0.0.1-beta.0 - Fix lint errors in integration tests (unused params)
Robdel12
force-pushed
the
rd/ember-sdk-testing
branch
from
798ba33 to
6ec5262
Compare
The launcher was hanging after tests completed because HTTP keep-alive connections prevented Node.js process exit. - Replace fetch() with node:http using Connection: close header - Add closeAllConnections() call in stopSnapshotServer() - Set Connection: close on snapshot server responses This matches the fix applied to the main CLI in commit 007a301.
Testem's Socket.IO round-trip for 'after-tests-complete' wasn't completing reliably with our Playwright launcher. Instead of waiting for SIGTERM (which never came), we now: 1. Detect the test framework (QUnit or Mocha) 2. Hook into its completion callback (QUnit.done or Mocha 'end' event) 3. Emit a console signal when tests finish 4. Listen for that signal in Playwright and trigger cleanup Also changed page.goto waitUntil from 'networkidle' to 'load' since Socket.IO keeps the network active.
Summary
Adds complete release infrastructure for the new Ember SDK to match other client SDKs.
release-ember-client.ymlworkflow with prerelease support (publishes to@betatag)blacksmith-4vcpu-ubuntu-2404with Playwright browser cachingvizzly runwithVIZZLY_EMBER_CLIENT_TOKENCHANGELOG.mdfor initial v0.0.1-beta.0 releaseLICENSEsymlink to root license0.0.1-beta.0CI Test Coverage
npm testnpm run test:integrationvizzly run "npx testem ci"Test plan