Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses test stability issues by refactoring how tests handle command-line arguments and changing the assertion library from assert.deepStrictEqual to expect().toBe(). The main changes involve removing direct manipulation of process.argv in tests and instead passing arguments through a new argv parameter in the create function.
Key changes:
- Replaced
assertwithexpectin test assertions for cleaner test syntax - Added an
argvparameter to thecreate()function to allow tests to pass arguments without modifyingprocess.argv - Updated
parseArgv()to accept a parameter instead of directly accessingprocess.argv
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/index.test.ts | Migrated from assert.deepStrictEqual to expect().toBe() for assertions |
| test/custom-tools.test.ts | Replaced process.argv manipulation with argv parameter; changed test to import from ../src and use rimraf for cleanup |
| test/agents.test.ts | Replaced process.argv manipulation with argv parameter; changed import to ../src |
| src/index.ts | Added argv parameter to create() function and updated parseArgv() to accept arguments; removed async from runCommand() |
| package.json | Added rimraf dependency for test file cleanup |
| pnpm-lock.yaml | Lock file updates for rimraf and its dependencies |
| .gitignore | Added test-temp-* pattern to ignore test output directories |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { fileURLToPath } from 'node:url'; | ||
| import { assert, beforeEach, expect, test } from '@rstest/core'; | ||
| import { create } from '../dist/index.js'; | ||
| import { create } from '../src'; |
There was a problem hiding this comment.
This test imports from ../src instead of ../dist/index.js, which is inconsistent with test/index.test.ts that imports from ../dist/index.js. Consider using the same import path across test files for consistency, especially since the built distribution is what will be published.
| import { create } from '../src'; | |
| import { create } from '../dist/index.js'; |
| const __dirname = path.dirname(fileURLToPath(import.meta.url)); | ||
| const testDir = path.join(__dirname, 'temp'); | ||
| const fixturesDir = path.join(__dirname, 'fixtures', 'basic'); | ||
| const testDir = path.join(fixturesDir, 'test-temp-output'); |
There was a problem hiding this comment.
The test output directory test-temp-output is now placed inside fixturesDir (fixtures/basic), which means test outputs will be created within the fixtures directory. This could lead to:
- Test artifacts polluting the fixtures directory
- Potential conflicts if fixtures are version-controlled
- Issues if fixtures directory is read-only
Consider placing the test output directory at the test root level instead:
const testDir = path.join(__dirname, 'test-temp-output');| const testDir = path.join(fixturesDir, 'test-temp-output'); | |
| const testDir = path.join(__dirname, 'test-temp-output'); |
| const __dirname = path.dirname(fileURLToPath(import.meta.url)); | ||
| const testDir = path.join(__dirname, 'temp'); | ||
| const fixturesDir = path.join(__dirname, 'fixtures', 'agents-md'); | ||
| const testDir = path.join(fixturesDir, 'test-temp-output'); |
There was a problem hiding this comment.
The test output directory test-temp-output is now placed inside fixturesDir (fixtures/agents-md), which means test outputs will be created within the fixtures directory. This could lead to:
- Test artifacts polluting the fixtures directory
- Potential conflicts if fixtures are version-controlled
- Issues if fixtures directory is read-only
Consider placing the test output directory at the test root level instead:
const testDir = path.join(__dirname, 'test-temp-output');| const testDir = path.join(fixturesDir, 'test-temp-output'); | |
| const testDir = path.join(__dirname, 'test-temp-output'); |
| value: 'custom-command', | ||
| label: 'Custom Command', | ||
| command: 'touch touched-by-command.txt', | ||
| command: `npx rimraf ${testFile}`, |
There was a problem hiding this comment.
Using npx rimraf ${testFile} with string interpolation could be problematic if the file path contains spaces or special characters. Consider using proper argument quoting or a more robust approach:
command: `npx rimraf "${testFile}"`Or better yet, use the rimraf package directly instead of relying on npx execution in tests.
| test('should run extra tool command', async () => { | ||
| const projectDir = path.join(testDir, 'extra-tool-command'); | ||
| const touchedFile = path.join(projectDir, 'touched-by-command.txt'); | ||
| const testFile = path.join(__dirname, 'node_modules', 'test.txt'); |
There was a problem hiding this comment.
The test file path path.join(__dirname, 'node_modules', 'test.txt') creates a file in the test directory's node_modules, which is unusual and potentially problematic. This could:
- Interfere with the actual node_modules structure
- Fail if node_modules doesn't exist or has restricted permissions
- Not be properly cleaned up by git (node_modules is typically gitignored)
Consider using the testDir path instead, which is properly cleaned up in the beforeEach hook:
const testFile = path.join(testDir, 'test.txt');| const testFile = path.join(__dirname, 'node_modules', 'test.txt'); | |
| const testFile = path.join(testDir, 'test.txt'); |
| import { beforeEach, expect, test } from '@rstest/core'; | ||
| import fse from 'fs-extra'; | ||
| import { create } from '../dist/index.js'; | ||
| import { create } from '../src'; |
There was a problem hiding this comment.
This test imports from ../src instead of ../dist/index.js, which is inconsistent with test/index.test.ts that imports from ../dist/index.js. Consider using the same import path across test files for consistency, especially since the built distribution is what will be published.
| import { create } from '../src'; | |
| import { create } from '../dist/index.js'; |
Summary
Fix unstable test cases.