feat: add tests

[mja00]

No description provided.

[claude]

Pull Request Review: feat: add tests

Overview

This PR adds a comprehensive test suite using Vitest with over 1,100 unit and integration tests. Great work on establishing test coverage! Here's my detailed feedback:

---

Positive Aspects

Test Coverage

• Excellent breadth: Tests cover utility functions (format, math, random, regex, string) and services (fishing)
• Good test structure: Clear separation between unit tests (tests/unit/) and integration tests (tests/integration/)
• Comprehensive scenarios: Tests cover edge cases, error handling, and boundary conditions
• Proper test organization: Each test file is well-organized with describe blocks for different methods

Configuration

• Solid Vitest setup: Good configuration with v8 coverage provider
• CI integration: Tests are properly integrated into GitHub Actions workflow
• ESLint overrides: Proper relaxation of rules for test files (disabling explicit return types makes sense for tests)
• TypeScript configuration: Separate tsconfig.test.json is appropriate

---

Issues & Concerns

1. Tests Don't Match Project Scope ⚠️

Severity: High

Looking at the project overview in CLAUDE.md, this is a "TypeScript-based Markov chain text generation system with OpenAI integration." However, the tests are for:

• A Discord fishing bot service (FishingService, catchables, rarities)
• Discord-specific utilities (mentions, tags, emojis)

Issue: The tests appear to be copied from a different project (likely a Discord fishing bot) and don't test the actual Markov chain functionality described in the project documentation.

Expected tests:

• MarkovChain.ts - state transitions, text generation, probability calculations
• OpenAI integration - thread management, response generation
• Utilities - text preprocessing, tokenization, file I/O

Recommendation:

• Either remove these tests and write tests for the actual Markov chain functionality
• Or update CLAUDE.md if the project scope has changed to include Discord fishing features

2. Missing Source Files for Tests

Severity: High

The tests reference source files that likely don't exist in this repository:

• /home/runner/work/markov.ts/markov.ts/tests/unit/services/fishing.service.test.ts:4 imports FishingService from ../../../src/services/fishing.service.js
• Tests import from src/enums/rarity.js, src/db/schema.js
• Tests for utilities that don't appear to be part of a Markov chain system

Problem: These tests will fail when run because the source files don't exist.

Recommendation: Run the tests locally before submitting. The tests should pass on a clean checkout of the repository.

3. Integration Test Mocking Concerns

Severity: Medium

In fishing-service.integration.test.ts:7-20:

vi.mock('../../src/services/database.service.js', () => ({
    getDb: vi.fn(),
}));

vi.mock('../../src/services/logger.js', () => ({
    Logger: {
        info: vi.fn(),
        warn: vi.fn(),
        error: vi.fn(),
        debug: vi.fn(),
    },
}));

Issue: These are labeled as "integration tests" but they mock external dependencies extensively. True integration tests should test actual database interactions and logging behavior.

Recommendation:

• Either rename to "service tests" (not integration tests)
• Or create true integration tests that use a test database instance
• Consider using an in-memory database for integration tests

4. Type Safety Issues

Severity: Medium

Several tests bypass TypeScript's type system:

fishing.service.test.ts:31:

const result = fishingService.getRarityName(999 as Rarity);

regex-utils.test.ts:135-141:

const result = RegexUtils.escapeRegex(undefined as any);
expect(result).toBeUndefined();

Issue: Using as any or as Rarity for invalid values defeats TypeScript's purpose. If the function should handle invalid inputs, it should be typed to accept them (e.g., number instead of Rarity).

Recommendation:

• Update function signatures to properly type what inputs are accepted
• If functions should only accept valid Rarity values, don't test invalid inputs - rely on TypeScript to prevent them
• For testing error cases, use proper types like Rarity | undefined if the function should handle those cases

5. Fragile Statistical Tests

Severity: Low

fishing-service.integration.test.ts:182-203:

expect(commonCount).toBeGreaterThan(500); // Should be around 600
expect(uncommonCount).toBeGreaterThan(200); // Should be around 300

random-utils.test.ts:179-185:

expect(count).toBeGreaterThan(iterations * 0.2);
expect(count).toBeLessThan(iterations * 0.47);

Issue: Statistical tests can be flaky and occasionally fail due to random variance, leading to intermittent CI failures.

Recommendation:

• Increase sample sizes for more stable bounds
• Consider using a seeded random number generator for deterministic tests
• Document why specific thresholds are chosen

6. Test File Naming Inconsistency

Severity: Low

Integration test: fishing-service.integration.test.ts
Unit test: fishing.service.test.ts

Issue: Inconsistent naming (dash vs dot separator)

Recommendation: Use consistent naming:

• fishing-service.unit.test.ts and fishing-service.integration.test.ts, OR
• fishing.service.test.ts and fishing-service.integration.test.ts

7. Small Code Change Unexplained

Severity: Low

src/utils/interaction-utils.ts:82-92 - Added withResponse: true to reply options

Issue: This change is not mentioned in the PR description and seems unrelated to adding tests.

Recommendation: Either:

• Explain this change in the PR description
• Move it to a separate PR if it's a bug fix or feature
• Revert if it was accidental

---

Recommendations

High Priority

1. Verify tests match the actual codebase - These tests appear to be for a different project
2. Ensure all tests pass - Run npm test on a clean checkout
3. Align tests with project scope - Test MarkovChain, OpenAI integration, not fishing mechanics

Medium Priority

4. Fix integration test architecture - Use real dependencies or rename as service tests
5. Improve type safety - Remove as any casts and properly type function inputs
6. Document the unrelated code change - Explain why withResponse: true was added

Low Priority

7. Stabilize statistical tests - Use seeded RNG or larger sample sizes
8. Standardize naming - Use consistent test file naming conventions

---

Security Concerns

✅ No security issues identified

---

Performance Considerations

• 1000+ test cases may slow down CI - consider splitting into separate workflows for unit vs integration tests
• Coverage configuration looks good with appropriate exclusions

---

Final Verdict

Request Changes - While the test infrastructure is well set up, the tests themselves don't appear to match the actual codebase (Markov chain system vs Discord fishing bot). Please verify the tests are for the correct project and that they pass when run.

Estimated effort to fix: Medium - If tests are from wrong project, significant rework needed. If source files exist but weren't included in PR, this could be a quick fix.

---

Good work on the testing infrastructure! Once the scope alignment is addressed, this will be a solid foundation for the project.