fix(commands): implement manual creation mode for add-task command

- Add support for --title/-t and --description/-d flags in add-task command - Fix validation for manual creation mode (title + description) - Implement proper testing for both prompt and manual creation modes - Update testing documentation with Commander.js testing best practices - Add guidance on handling variable hoisting and module initialization issues Changeset: brave-doors-open.md
2025-04-09 18:18:13 -04:00
parent 709ea63350
commit 12519946b4
15 changed files with 1539 additions and 460 deletions
--- a/.cursor/rules/tests.mdc
+++ b/.cursor/rules/tests.mdc
@@ -90,6 +90,122 @@ describe('Feature or Function Name', () => {
 });
 ```

+## Commander.js Command Testing Best Practices
+
+When testing CLI commands built with Commander.js, several special considerations must be made to avoid common pitfalls:
+
+- **Direct Action Handler Testing**
+  - ✅ **DO**: Test the command action handlers directly rather than trying to mock the entire Commander.js chain
+  - ✅ **DO**: Create simplified test-specific implementations of command handlers that match the original behavior
+  - ✅ **DO**: Explicitly handle all options, including defaults and shorthand flags (e.g., `-p` for `--prompt`)
+  - ✅ **DO**: Include null/undefined checks in test implementations for parameters that might be optional
+  - ✅ **DO**: Use fixtures from `tests/fixtures/` for consistent sample data across tests
+  
+  ```javascript
+  // ✅ DO: Create a simplified test version of the command handler
+  const testAddTaskAction = async (options) => {
+    options = options || {}; // Ensure options aren't undefined
+    
+    // Validate parameters
+    const isManualCreation = options.title && options.description;
+    const prompt = options.prompt || options.p; // Handle shorthand flags
+    
+    if (!prompt && !isManualCreation) {
+      throw new Error('Expected error message');
+    }
+    
+    // Call the mocked task manager
+    return mockTaskManager.addTask(/* parameters */);
+  };
+  
+  test('should handle required parameters correctly', async () => {
+    // Call the test implementation directly
+    await expect(async () => {
+      await testAddTaskAction({ file: 'tasks.json' });
+    }).rejects.toThrow('Expected error message');
+  });
+  ```
+
+- **Commander Chain Mocking (If Necessary)**
+  - ✅ **DO**: Mock ALL chainable methods (`option`, `argument`, `action`, `on`, etc.)
+  - ✅ **DO**: Return `this` (or the mock object) from all chainable method mocks
+  - ✅ **DO**: Remember to mock not only the initial object but also all objects returned by methods
+  - ✅ **DO**: Implement a mechanism to capture the action handler for direct testing
+  
+  ```javascript
+  // If you must mock the Commander.js chain:
+  const mockCommand = {
+    command: jest.fn().mockReturnThis(),
+    description: jest.fn().mockReturnThis(),
+    option: jest.fn().mockReturnThis(),
+    argument: jest.fn().mockReturnThis(), // Don't forget this one
+    action: jest.fn(fn => { 
+      actionHandler = fn; // Capture the handler for testing
+      return mockCommand; 
+    }),
+    on: jest.fn().mockReturnThis() // Don't forget this one
+  };
+  ```
+
+- **Parameter Handling**
+  - ✅ **DO**: Check for both main flag and shorthand flags (e.g., `prompt` and `p`)
+  - ✅ **DO**: Handle parameters like Commander would (comma-separated lists, etc.)
+  - ✅ **DO**: Set proper default values as defined in the command
+  - ✅ **DO**: Validate that required parameters are actually required in tests
+  
+  ```javascript
+  // Parse dependencies like Commander would
+  const dependencies = options.dependencies 
+    ? options.dependencies.split(',').map(id => id.trim())
+    : [];
+  ```
+
+- **Environment and Session Handling**
+  - ✅ **DO**: Properly mock session objects when required by functions
+  - ✅ **DO**: Reset environment variables between tests if modified
+  - ✅ **DO**: Use a consistent pattern for environment-dependent tests
+  
+  ```javascript
+  // Session parameter mock pattern
+  const sessionMock = { session: process.env };
+  
+  // In test:
+  expect(mockAddTask).toHaveBeenCalledWith(
+    expect.any(String),
+    'Test prompt',
+    [],
+    'medium',
+    sessionMock,
+    false,
+    null,
+    null
+  );
+  ```
+
+- **Common Pitfalls to Avoid**
+  - ❌ **DON'T**: Try to use the real action implementation without proper mocking
+  - ❌ **DON'T**: Mock Commander partially - either mock it completely or test the action directly
+  - ❌ **DON'T**: Forget to handle optional parameters that may be undefined
+  - ❌ **DON'T**: Neglect to test shorthand flag functionality (e.g., `-p`, `-r`)
+  - ❌ **DON'T**: Create circular dependencies in your test mocks
+  - ❌ **DON'T**: Access variables before initialization in your test implementations
+  - ❌ **DON'T**: Include actual command execution in unit tests
+  - ❌ **DON'T**: Overwrite the same file path in multiple tests
+  
+  ```javascript
+  // ❌ DON'T: Create circular references in mocks
+  const badMock = {
+    method: jest.fn().mockImplementation(() => badMock.method())
+  };
+  
+  // ❌ DON'T: Access uninitialized variables
+  const badImplementation = () => {
+    const result = uninitialized;
+    let uninitialized = 'value';
+    return result;
+  };
+  ```
+
 ## Jest Module Mocking Best Practices

 - **Mock Hoisting Behavior**
@@ -570,7 +686,7 @@ npm test -- -t "pattern to match"
    Anthropic: jest.fn().mockImplementation(() => ({
      messages: {
        create: jest.fn().mockResolvedValue({
-          content: [{ type: 'text', text: JSON.stringify({ title: "Test Task" }) }]
+          content: [{ type: 'text', text: 'Mocked AI response' }]
        })
      }
    }))
@@ -680,4 +796,107 @@ npm test -- -t "pattern to match"

 See [tests/README.md](mdc:tests/README.md) for more details on the testing approach.

-Refer to [jest.config.js](mdc:jest.config.js) for Jest configuration options. 
+Refer to [jest.config.js](mdc:jest.config.js) for Jest configuration options.
+
+## Variable Hoisting and Module Initialization Issues
+
+When testing ES modules or working with complex module imports, you may encounter variable hoisting and initialization issues. These can be particularly tricky to debug and often appear as "Cannot access 'X' before initialization" errors.
+
+- **Understanding Module Initialization Order**
+  - ✅ **DO**: Declare and initialize global variables at the top of modules
+  - ✅ **DO**: Use proper function declarations to avoid hoisting issues
+  - ✅ **DO**: Initialize variables before they are referenced, especially in imported modules
+  - ✅ **DO**: Be aware that imports are hoisted to the top of the file
+  
+  ```javascript
+  // ✅ DO: Define global state variables at the top of the module
+  let silentMode = false; // Declare and initialize first
+  
+  const CONFIG = { /* configuration */ };
+  
+  function isSilentMode() {
+    return silentMode; // Reference variable after it's initialized
+  }
+  
+  function log(level, message) {
+    if (isSilentMode()) return; // Use the function instead of accessing variable directly
+    // ...
+  }
+  ```
+
+- **Testing Modules with Initialization-Dependent Functions**
+  - ✅ **DO**: Create test-specific implementations that initialize all variables correctly
+  - ✅ **DO**: Use factory functions in mocks to ensure proper initialization order
+  - ✅ **DO**: Be careful with how you mock or stub functions that depend on module state
+  
+  ```javascript
+  // ✅ DO: Test-specific implementation that avoids initialization issues
+  const testLog = (level, ...args) => {
+    // Local implementation with proper initialization
+    const isSilent = false; // Explicit initialization
+    if (isSilent) return;
+    // Test implementation...
+  };
+  ```
+
+- **Common Hoisting-Related Errors to Avoid**
+  - ❌ **DON'T**: Reference variables before their declaration in module scope
+  - ❌ **DON'T**: Create circular dependencies between modules
+  - ❌ **DON'T**: Rely on variable initialization order across module boundaries
+  - ❌ **DON'T**: Define functions that use hoisted variables before they're initialized
+  
+  ```javascript
+  // ❌ DON'T: Create reference-before-initialization patterns
+  function badFunction() {
+    if (silentMode) { /* ... */ } // ReferenceError if silentMode is declared later
+  }
+  
+  let silentMode = false;
+  
+  // ❌ DON'T: Create cross-module references that depend on initialization order
+  // module-a.js
+  import { getSetting } from './module-b.js';
+  export const config = { value: getSetting() };
+  
+  // module-b.js
+  import { config } from './module-a.js';
+  export function getSetting() {
+    return config.value; // Circular dependency causing initialization issues
+  }
+  ```
+
+- **Dynamic Imports as a Solution**
+  - ✅ **DO**: Use dynamic imports (`import()`) to avoid initialization order issues
+  - ✅ **DO**: Structure modules to avoid circular dependencies that cause initialization issues
+  - ✅ **DO**: Consider factory functions for modules with complex state
+  
+  ```javascript
+  // ✅ DO: Use dynamic imports to avoid initialization issues
+  async function getTaskManager() {
+    return import('./task-manager.js');
+  }
+  
+  async function someFunction() {
+    const taskManager = await getTaskManager();
+    return taskManager.someMethod();
+  }
+  ```
+
+- **Testing Approach for Modules with Initialization Issues**
+  - ✅ **DO**: Create self-contained test implementations rather than using real implementations
+  - ✅ **DO**: Mock dependencies at module boundaries instead of trying to mock deep dependencies
+  - ✅ **DO**: Isolate module-specific state in tests
+  
+  ```javascript
+  // ✅ DO: Create isolated test implementation instead of reusing module code
+  test('should log messages when not in silent mode', () => {
+    // Local test implementation instead of importing from module
+    const testLog = (level, message) => {
+      if (false) return; // Always non-silent for this test
+      mockConsole(level, message);
+    };
+    
+    testLog('info', 'test message');
+    expect(mockConsole).toHaveBeenCalledWith('info', 'test message');
+  });
+  ```