Skip to content

Conversation

@bensabic
Copy link

@bensabic bensabic commented Dec 2, 2025

  • Install jest as dev dependency
  • Add test script to package.json
  • Create unit tests for storage.js
  • Verify tests pass

@bentossell @factory-ben

- Install jest as dev dependency
- Add test script to package.json
- Create unit tests for storage.js
- Migrate from npm to pnpm
- Update all documentation to reference pnpm
- Update GitHub Actions to use pnpm
- Verify tests pass
@bensabic bensabic changed the title feat: add Jest testing infrastructure and initial storage tests feat: add Jest tests and migrate to pnpm Dec 2, 2025
@factory-ben
Copy link
Owner

@droid review

@github-actions
Copy link
Contributor

github-actions bot commented Dec 2, 2025

Code Review: feat: add Jest tests and migrate to pnpm

✅ Overall Assessment

This is a solid PR that adds Jest testing infrastructure and migrates the package manager from npm to pnpm. The tests pass and the changes are well-organized.

👍 What's Good

  1. Test Coverage: Good coverage of the storage.js module with tests for filterNewItems, saveFeed, and initStorage
  2. Clean Mocking: Proper use of jest.mock for fs operations
  3. Documentation Updates: All docs (AGENTS.md, CONTRIBUTING.md, README.md) updated consistently from npm to pnpm
  4. CI/CD: GitHub Actions workflow properly updated to use pnpm with pnpm/action-setup@v4

🔧 Suggestions for Improvement

  1. Unused imports in test file (minor):

    • path is imported but never used
    • saveSeen and loadFeed are imported but not tested
  2. Missing test for loadFeed: The function is imported but has no tests. Consider adding a test case.

  3. Missing test for loadSeen: This function isn't even imported but should be tested.

  4. Missing test for saveSeen: Imported but not tested.

  5. Edge case coverage: Consider adding tests for:

    • Empty array input to filterNewItems
    • saveFeed with less than 100 items (to ensure no trimming issues)
    • Error handling paths in storage functions

📝 Code Snippet Issue

In the test file, there's an unused import that could be cleaned up:

// Line 1-2: fs and path are imported but only fs is used via jest.mock
const fs = require('fs').promises;  // Used (for mock reference)
const path = require('path');        // Not used - can be removed

✅ Approval

The PR is ready to merge as-is. The suggestions above are optional improvements that could be addressed in a follow-up PR.

All 5 tests pass:

  • filterNewItems - 3 tests ✓
  • saveFeed - 1 test ✓
  • initStorage - 1 test ✓

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants