fix: setup proper tests for core and plugin-openapi packages (Fixes #52)

[michaelbe812]

Summary

• Added comprehensive unit tests for packages/core and packages/plugin-openapi
• Significantly improved test coverage for critical functionality

Changes Made

Core Package Tests

• plugin-loader.spec.ts - Tests for dynamic plugin loading, caching, and error handling
• registry.spec.ts - Tests for plugin registry singleton pattern and operations
• base-generator.spec.ts - Tests for output directory cleaning functionality
• auto-installer.spec.ts - Tests for package manager detection and installation
• errors.spec.ts - Tests for custom error classes and inheritance

Plugin-OpenAPI Package Tests

• openapi-tools-generator.spec.ts - Tests for OpenAPI generation with single/multiple specs
• build-command.spec.ts - Tests for command argument building with all options

Test Results

• All tests passing
• Core package: 71 tests passing
• Plugin-OpenAPI package: 32 tests passing

Next Steps

While this PR significantly improves test coverage, additional tests could be added for generator functions and utility functions in follow-up PRs.

Fixes #52

[michaelbe812]

Code Review Summary — PR #53

Title: fix: setup proper tests for core and plugin-openapi packages (Fixes #52)
Branch: fix/issue-52-test-coverage
Scope: 7 new test files, ~1645 insertions, no production code changes

What I Reviewed

• New Jest specs in packages/core and packages/plugin-openapi:
  • core: auto-installer.spec.ts, base-generator.spec.ts, errors.spec.ts, plugin-loader.spec.ts, registry.spec.ts
  • plugin-openapi: openapi-tools-generator.spec.ts, utils/build-command.spec.ts
• Diff stat: 7 files changed, 1645 insertions (+), 0 deletions

Validation

• Tests: nx run-many -t test -p core,plugin-openapi → all suites pass (71 tests core, 32 tests plugin-openapi)
• Lint: core:lint reports 6 errors (no-explicit-any in spec files); plugin-openapi has no lint target configured

Strengths

• Broad coverage of critical paths: error types, registry singleton, plugin loading permutations, command arg builder, and generator process lifecycle.
• Clear mocking of child_process/fs and dynamic import cases; good assertions on error messaging and behavior.
• Nice multi-spec (map) handling tests for the OpenAPI generator, including failure propagation.

Key Findings / Risks

• BaseGenerator path safety: cleanOutput uses join(ctx.root, relOutputPath). If relOutputPath is absolute (e.g., /tmp), join ignores ctx.root, risking deletions outside the workspace. Tests don’t cover truly absolute inputs; one test expects "/workspace/" for "/", which isn’t how path.join behaves on POSIX.
  • Suggestion: guard outputs to remain within ctx.root (e.g., resolve then verify relative(path, ctx.root) doesn’t start with ..) and reject absolute inputs.
• Auto-installer empty installs: installPackages([]) builds commands like "pnpm add -D " and attempts execution. Test codifies this, but it’s better to no-op early when pkgs.length === 0.
• Plugin loader fallback: Fallback candidates include TS source (packages/plugin-openapi/src/index.ts), which Node can’t import directly. Prefer dist JS paths only and add a dedicated fallback test that verifies the candidate search order.
• Lint gaps: plugin-openapi lacks a lint target; core spec files violate no-explicit-any in a few spots (test scaffolding).
• Potential test brittleness: openapi-tools-generator.spec.ts mocks spawn to return the same EventEmitter across calls; for multi-spec runs, consider returning a fresh emitter per call to better simulate separate child processes.

Suggestions

• Add path guard in BaseGenerator.cleanOutput and update tests to reflect absolute-path rejection.
• Short-circuit installPackages on empty arrays; update test accordingly.
• Tighten plugin-loader fallback to dist outputs and add tests for fallback resolution (and for non-Node-loadable TS paths).
• Add lint target to plugin-openapi and address no-explicit-any in specs (type aliases or unknown with narrowing).
• Consider Jest coverage thresholds in each package to lock in these gains.

Verdict

• Tests are a solid improvement and pass locally. Address the noted path-safety and lint items before merging, or track them as follow-ups if you want to land coverage first.

If you want, I can open a follow-up PR to implement the path guard and empty-install short-circuit with updated tests.

[graphite-app]

Security Issue: The test reveals a potential command injection vulnerability. When an empty package array is passed to installPackages(), it generates a malformed command with a trailing space: pnpm add -D .

This could lead to:

1. Command execution errors
2. Unexpected behavior in production code
3. Potential security risks if user input is involved

Consider either:

• Adding a guard clause to skip installation when the package array is empty
• Trimming the command string before execution
• Modifying the test to verify proper handling of empty arrays

The implementation should ensure commands are properly formatted before being passed to execSync().

Suggested change

	expect(execSync).toHaveBeenCalledWith('pnpm add -D ', {
	expect(execSync).not.toHaveBeenCalled();

Spotted by Diamond



Is this helpful? React 👍 or 👎 to let us know.

[graphite-app]

Security Vulnerability: The test reveals that cleanOutput() will delete the entire workspace root directory when given an empty path. This is dangerous behavior that could lead to catastrophic data loss in production.

The implementation should validate that relOutputPath is not empty or contains only path separators before performing the recursive deletion. Consider adding a guard clause that throws an error or returns early when an empty/invalid path is provided.

// Example fix
if (!relOutputPath || relOutputPath.trim() === '' || relOutputPath === '/' || relOutputPath === '.') {
  throw new Error('Cannot clean empty or root output path for safety reasons');
}

Spotted by Diamond



Is this helpful? React 👍 or 👎 to let us know.

[graphite-app]

The test has a potential race condition. Using setTimeout with different delays (0ms vs 10ms) creates timing-dependent behavior that could lead to inconsistent test results across different environments. Consider using a more deterministic approach to simulate sequential process completion, such as chaining promises or using jest.advanceTimersByTime() with mocked timers. This would make the test more reliable and better at detecting actual concurrency issues in the generator implementation.

Suggested change

	process.nextTick(() => mockChildProcess.emit('close', 0));
	setTimeout(() => mockChildProcess.emit('close', 1), 10);
	jest.useFakeTimers();
	process.nextTick(() => mockChildProcess.emit('close', 0));
	setTimeout(() => mockChildProcess.emit('close', 1), 10);
	jest.runAllTimers();

Spotted by Diamond



Is this helpful? React 👍 or 👎 to let us know.