-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat(cli): add --no-color flag and fix color propagation to workers #1163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -106,7 +106,6 @@ describe('processConcurrency', () => { | |
| }, | ||
| env: expect.objectContaining({ | ||
| REPOMIX_LOG_LEVEL: '2', | ||
| FORCE_COLOR: expect.any(String), | ||
| TERM: expect.any(String), | ||
| }), | ||
| }); | ||
|
|
@@ -167,4 +166,110 @@ describe('processConcurrency', () => { | |
| expect(taskRunner).toHaveProperty('cleanup'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('color propagation to worker processes', () => { | ||
| beforeEach(() => { | ||
| vi.mocked(os).availableParallelism = vi.fn().mockReturnValue(4); | ||
| vi.mocked(Tinypool).mockImplementation(function (this: unknown) { | ||
| (this as Record<string, unknown>).run = vi.fn(); | ||
| (this as Record<string, unknown>).destroy = vi.fn(); | ||
| return this as Tinypool; | ||
| }); | ||
| }); | ||
|
|
||
| it('should propagate NO_COLOR to worker when NO_COLOR env is set', () => { | ||
| const originalNoColor = process.env.NO_COLOR; | ||
| process.env.NO_COLOR = '1'; | ||
| try { | ||
| createWorkerPool({ numOfTasks: 100, workerType: 'fileProcess', runtime: 'child_process' }); | ||
|
|
||
| expect(Tinypool).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| env: expect.objectContaining({ | ||
| NO_COLOR: '1', | ||
| }), | ||
| }), | ||
| ); | ||
| } finally { | ||
| if (originalNoColor === undefined) { | ||
| delete process.env.NO_COLOR; | ||
| } else { | ||
| process.env.NO_COLOR = originalNoColor; | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| it('should propagate NO_COLOR to worker when --no-color is in argv', () => { | ||
| const originalArgv = process.argv; | ||
| process.argv = [...originalArgv, '--no-color']; | ||
| try { | ||
| createWorkerPool({ numOfTasks: 100, workerType: 'fileProcess', runtime: 'child_process' }); | ||
|
|
||
| expect(Tinypool).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| env: expect.objectContaining({ | ||
| NO_COLOR: '1', | ||
| }), | ||
| }), | ||
| ); | ||
| } finally { | ||
| process.argv = originalArgv; | ||
| } | ||
| }); | ||
|
|
||
| it('should set FORCE_COLOR when colors are not disabled', () => { | ||
| const originalNoColor = process.env.NO_COLOR; | ||
| delete process.env.NO_COLOR; | ||
| const originalArgv = process.argv; | ||
| process.argv = originalArgv.filter((arg) => arg !== '--no-color'); | ||
| try { | ||
| createWorkerPool({ numOfTasks: 100, workerType: 'fileProcess', runtime: 'child_process' }); | ||
|
|
||
| expect(Tinypool).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| env: expect.objectContaining({ | ||
| FORCE_COLOR: expect.any(String), | ||
| }), | ||
| }), | ||
| ); | ||
| } finally { | ||
| if (originalNoColor !== undefined) { | ||
| process.env.NO_COLOR = originalNoColor; | ||
| } | ||
| process.argv = originalArgv; | ||
| } | ||
| }); | ||
|
|
||
| it('should not leak FORCE_COLOR when NO_COLOR is enabled', () => { | ||
| const originalNoColor = process.env.NO_COLOR; | ||
| const originalForceColor = process.env.FORCE_COLOR; | ||
| process.env.NO_COLOR = '1'; | ||
| process.env.FORCE_COLOR = '1'; | ||
| try { | ||
| createWorkerPool({ numOfTasks: 100, workerType: 'fileProcess', runtime: 'child_process' }); | ||
|
|
||
| expect(Tinypool).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| env: expect.objectContaining({ | ||
| NO_COLOR: '1', | ||
| }), | ||
| }), | ||
| ); | ||
|
|
||
| const callArgs = vi.mocked(Tinypool).mock.calls.at(-1)?.[0]; | ||
| expect(callArgs?.env?.FORCE_COLOR).toBeUndefined(); | ||
| } finally { | ||
| if (originalNoColor === undefined) { | ||
| delete process.env.NO_COLOR; | ||
| } else { | ||
| process.env.NO_COLOR = originalNoColor; | ||
| } | ||
| if (originalForceColor === undefined) { | ||
| delete process.env.FORCE_COLOR; | ||
| } else { | ||
| process.env.FORCE_COLOR = originalForceColor; | ||
| } | ||
| } | ||
| }); | ||
| }); | ||
|
Comment on lines
+170
to
+274
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To improve test isolation and reduce repetitive code, consider managing the global state (
Comment on lines
+170
to
+274
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing test coverage for The three new tests cover the positive propagation cases, but there is no test verifying that a pre-existing Add a fourth test: 🧪 Suggested additional test+ it('should not carry FORCE_COLOR into worker when NO_COLOR is active', () => {
+ const originalNoColor = process.env.NO_COLOR;
+ const originalForceColor = process.env.FORCE_COLOR;
+ process.env.NO_COLOR = '1';
+ process.env.FORCE_COLOR = '1'; // simulate CI/parent setting FORCE_COLOR
+ try {
+ createWorkerPool({ numOfTasks: 100, workerType: 'fileProcess', runtime: 'child_process' });
+
+ const callArgs = vi.mocked(Tinypool).mock.calls[0][0] as { env: Record<string, string> };
+ expect(callArgs.env.NO_COLOR).toBe('1');
+ expect(callArgs.env.FORCE_COLOR).toBeUndefined();
+ } finally {
+ if (originalNoColor === undefined) delete process.env.NO_COLOR;
+ else process.env.NO_COLOR = originalNoColor;
+ if (originalForceColor === undefined) delete process.env.FORCE_COLOR;
+ else process.env.FORCE_COLOR = originalForceColor;
+ }
+ });🤖 Prompt for AI Agents |
||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is a good start, but it only indirectly verifies the behavior by checking the final
console.logoutput. To make the test more robust and specific, you could spy onutil.inspectand assert that it's called with the correctcolorsoption. This would directly test the logic informatArgs. You could also test the case where colors are disabled to ensureutil.inspectis called withcolors: false.