Merge pull request #350 from recca0120/refactor/use-children-replace

refactor: eliminate duplicate data, simplify dependencies, fix singleton issues

Author: recca0120

.github/workflows/e2e.yml

@@ -10,9 +10,8 @@ jobs:
   e2e:
     runs-on: ${{ matrix.os }}
     strategy:
-      fail-fast: false
       matrix:
-        os: [macos-latest, ubuntu-latest, windows-latest]
+        os: [ubuntu-latest, macos-latest, windows-latest]
     name: e2e (${{ matrix.os }})
     defaults:
       run:

.github/workflows/tests.yml

@@ -11,9 +11,8 @@ jobs:
     runs-on: ${{ matrix.os }}
 
     strategy:
-      fail-fast: false
       matrix:
-        os: [macos-latest, ubuntu-latest, windows-latest]
+        os: [ubuntu-latest, macos-latest, windows-latest]
         php: ['7.4', '8.2', '8.4']
         node: ['20']
 

src/Commands/TestCommandRegistry.ts

@@ -100,9 +100,11 @@ export class TestCommandRegistry {
         }
 
         const tests = findTests(ctx, uri);
-        if (tests.length > 0) {
-            await this.run(tests);
+        if (tests.length === 0) {
+            return;
         }
+
+        await this.run(tests);
     }
 
     private async run(include: readonly TestItem[] | undefined) {

src/Observers/ErrorDialogObserver.ts

@@ -1,11 +1,8 @@
-import { inject, injectable } from 'inversify';
 import { window } from 'vscode';
-import { Configuration } from '../Configuration';
 import type { IConfiguration, TestRunnerObserver } from '../PHPUnit';
 
-@injectable()
 export class ErrorDialogObserver implements TestRunnerObserver {
-    constructor(@inject(Configuration) private configuration: IConfiguration) {}
+    constructor(private configuration: IConfiguration) {}
 
     async error(error: string) {
         if (error.includes('Pest\\Exceptions\\InvalidPestCommand')) {

src/Observers/OutputChannelObserver.test.ts

@@ -8,7 +8,7 @@ import { OutputChannelObserver, OutputFormatter } from './index';
 import { PrettyPrinter } from './Printers';
 
 describe('OutputChannelObserver clear behavior', () => {
-    const createObserver = (config: Record<string, unknown> = {}) => {
+    const createObserver = (config: Record<string, unknown> = {}, request: TestRunRequest = { continuous: false } as TestRunRequest) => {
         const outputChannel = vscode.window.createOutputChannel('phpunit');
         const configuration = new Configuration({
             clearOutputOnRun: true,
@@ -19,6 +19,7 @@ describe('OutputChannelObserver clear behavior', () => {
             outputChannel,
             configuration,
             new PrettyPrinter(new PHPUnitXML()),
+            request,
         );
 
         return { observer, outputChannel };
@@ -30,7 +31,6 @@ describe('OutputChannelObserver clear behavior', () => {
 
     it('clears once for multiple processes in the same request', () => {
         const { observer, outputChannel } = createObserver();
-        observer.setRequest({ continuous: false } as TestRunRequest);
 
         observer.run(createBuilder('command-1'));
         observer.run(createBuilder('command-2'));
@@ -39,30 +39,33 @@ describe('OutputChannelObserver clear behavior', () => {
     });
 
     it('should show output channel when continuous is undefined (standard run)', () => {
-        const { observer, outputChannel } = createObserver({ showAfterExecution: 'always' });
-        observer.setRequest({} as TestRunRequest);
+        const { observer, outputChannel } = createObserver({ showAfterExecution: 'always' }, {} as TestRunRequest);
 
         observer.run(createBuilder('command-1'));
 
         expect(outputChannel.show).toHaveBeenCalled();
     });
 
     it('should not show output channel when continuous is true', () => {
-        const { observer, outputChannel } = createObserver({ showAfterExecution: 'always' });
-        observer.setRequest({ continuous: true } as TestRunRequest);
+        const { observer, outputChannel } = createObserver({ showAfterExecution: 'always' }, { continuous: true } as TestRunRequest);
 
         observer.run(createBuilder('command-1'));
 
         expect(outputChannel.show).not.toHaveBeenCalled();
     });
 
-    it('clears again for a new request', () => {
-        const { observer, outputChannel } = createObserver();
-        observer.setRequest({ continuous: false } as TestRunRequest);
-        observer.run(createBuilder('command-1'));
+    it('each observer instance clears independently', () => {
+        const { outputChannel } = createObserver();
+        const config = {
+            clearOutputOnRun: true,
+            showAfterExecution: 'onFailure',
+        };
+        const configuration = new Configuration(config);
+        const observer1 = new OutputChannelObserver(outputChannel, configuration, new PrettyPrinter(new PHPUnitXML()), { continuous: false } as TestRunRequest);
+        const observer2 = new OutputChannelObserver(outputChannel, configuration, new PrettyPrinter(new PHPUnitXML()), { continuous: false } as TestRunRequest);
 
-        observer.setRequest({ continuous: false } as TestRunRequest);
-        observer.run(createBuilder('command-2'));
+        observer1.run(createBuilder('command-1'));
+        observer2.run(createBuilder('command-2'));
 
         expect(outputChannel.clear).toHaveBeenCalledTimes(2);
     });
@@ -92,8 +95,8 @@ describe.each(detectPhpUnitStubs())('OutputChannelObserver on $name (PHPUnit $ph
             outputChannel,
             configuration,
             new PrettyPrinter(new PHPUnitXML()),
+            { continuous: false } as TestRunRequest,
         );
-        observer.setRequest({ continuous: false } as TestRunRequest);
         testRunner.observe(observer);
     });
 

src/Observers/OutputChannelObserver.ts

@@ -1,6 +1,4 @@
-import { inject, injectable } from 'inversify';
 import type { OutputChannel, TestRunRequest } from 'vscode';
-import { Configuration } from '../Configuration';
 import type {
     IConfiguration,
     ProcessBuilder,
@@ -19,32 +17,25 @@ import type {
     TestSuiteStarted,
     TestVersion,
 } from '../PHPUnit';
-import { TYPES } from '../types';
-import { OutputFormatter } from './Printers';
+import type { OutputFormatter } from './Printers';
 
 enum ShowOutputState {
     always = 'always',
     onFailure = 'onFailure',
     never = 'never',
 }
 
-@injectable()
 export class OutputChannelObserver implements TestRunnerObserver {
     private lastCommand = '';
     private hasClearedCurrentRequest = false;
-    private request!: TestRunRequest;
 
     constructor(
-        @inject(TYPES.OutputChannel) private outputChannel: OutputChannel,
-        @inject(Configuration) private configuration: IConfiguration,
-        @inject(OutputFormatter) private outputFormatter: OutputFormatter,
+        private outputChannel: OutputChannel,
+        private configuration: IConfiguration,
+        private outputFormatter: OutputFormatter,
+        private request: TestRunRequest,
     ) {}
 
-    setRequest(request: TestRunRequest) {
-        this.request = request;
-        this.hasClearedCurrentRequest = false;
-    }
-
     run(builder: ProcessBuilder): void {
         this.clearOutputOnRun();
         this.showOutputChannel(ShowOutputState.always);

src/Observers/Printers/CollisionPrinter.ts

@@ -1,4 +1,3 @@
-import { injectable, injectFromBase } from 'inversify';
 import {
     EOL,
     TeamcityEvent,
@@ -10,8 +9,6 @@ import {
 import { OutputFormatter } from './OutputFormatter';
 import { readSourceSnippet } from './SourceFileReader';
 
-@injectable()
-@injectFromBase()
 export class CollisionPrinter extends OutputFormatter {
     private errors: TestFailed[] = [];
 

src/Observers/Printers/OutputFormatter.ts

@@ -1,4 +1,3 @@
-import { inject, injectable } from 'inversify';
 import {
     EOL,
     PHPUnitXML,
@@ -18,7 +17,6 @@ import {
 } from '../../PHPUnit';
 import { OutputBuffer } from './OutputBuffer';
 
-@injectable()
 export abstract class OutputFormatter {
     protected outputBuffer = new OutputBuffer();
     protected messages = new Map<TeamcityEvent, string[]>([
@@ -28,7 +26,7 @@ export abstract class OutputFormatter {
         [TeamcityEvent.testIgnored, ['➖', 'IGNORED']],
     ]);
 
-    constructor(@inject(PHPUnitXML) protected phpUnitXML: PHPUnitXML) {}
+    constructor(protected phpUnitXML: PHPUnitXML) {}
 
     static fileFormat(file: string, line: number) {
         return `${file}:${line}`;

src/Observers/TestRunnerObserverFactory.test.ts

@@ -0,0 +1,50 @@
+import { beforeEach, describe, expect, it, vi } from 'vitest';
+import type { OutputChannel, TestItem, TestRun, TestRunRequest } from 'vscode';
+import { Configuration } from '../Configuration';
+import { PHPUnitXML, type TestDefinition } from '../PHPUnit';
+import { OutputChannelObserver } from './OutputChannelObserver';
+import { TestResultObserver } from './TestResultObserver';
+import { TestRunnerObserverFactory } from './TestRunnerObserverFactory';
+
+describe('TestRunnerObserverFactory', () => {
+    let factory: TestRunnerObserverFactory;
+    let outputChannel: OutputChannel;
+
+    beforeEach(() => {
+        outputChannel = {
+            append: vi.fn(),
+            appendLine: vi.fn(),
+            clear: vi.fn(),
+            show: vi.fn(),
+        } as unknown as OutputChannel;
+        const configuration = { get: vi.fn() } as unknown as Configuration;
+        const phpUnitXML = new PHPUnitXML();
+        factory = new TestRunnerObserverFactory(outputChannel, configuration, phpUnitXML);
+    });
+
+    it('should create observers including OutputChannelObserver and TestResultObserver', () => {
+        const queue = new Map<TestDefinition, TestItem>();
+        const testRun = { enqueued: vi.fn() } as unknown as TestRun;
+        const request = { continuous: false } as TestRunRequest;
+
+        const observers = factory.create(queue, testRun, request);
+
+        expect(observers.length).toBe(3);
+        expect(observers.some((o) => o instanceof TestResultObserver)).toBe(true);
+        expect(observers.some((o) => o instanceof OutputChannelObserver)).toBe(true);
+    });
+
+    it('should create new instances for each call', () => {
+        const queue = new Map<TestDefinition, TestItem>();
+        const testRun = { enqueued: vi.fn() } as unknown as TestRun;
+        const request = { continuous: false } as TestRunRequest;
+
+        const observers1 = factory.create(queue, testRun, request);
+        const observers2 = factory.create(queue, testRun, request);
+
+        const output1 = observers1.find((o) => o instanceof OutputChannelObserver);
+        const output2 = observers2.find((o) => o instanceof OutputChannelObserver);
+
+        expect(output1).not.toBe(output2);
+    });
+});

src/Observers/TestRunnerObserverFactory.ts

@@ -0,0 +1,30 @@
+import { inject, injectable } from 'inversify';
+import type { OutputChannel, TestItem, TestRun, TestRunRequest } from 'vscode';
+import { Configuration } from '../Configuration';
+import { PHPUnitXML, type IConfiguration, type TestDefinition, type TestRunnerObserver } from '../PHPUnit';
+import { TYPES } from '../types';
+import { ErrorDialogObserver } from './ErrorDialogObserver';
+import { OutputChannelObserver } from './OutputChannelObserver';
+import { CollisionPrinter } from './Printers';
+import { TestResultObserver } from './TestResultObserver';
+
+@injectable()
+export class TestRunnerObserverFactory {
+    constructor(
+        @inject(TYPES.OutputChannel) private outputChannel: OutputChannel,
+        @inject(Configuration) private configuration: IConfiguration,
+        @inject(PHPUnitXML) private phpUnitXML: PHPUnitXML,
+    ) {}
+
+    create(
+        queue: Map<TestDefinition, TestItem>,
+        testRun: TestRun,
+        request: TestRunRequest,
+    ): TestRunnerObserver[] {
+        return [
+            new TestResultObserver(queue, testRun),
+            new OutputChannelObserver(this.outputChannel, this.configuration, new CollisionPrinter(this.phpUnitXML), request),
+            new ErrorDialogObserver(this.configuration),
+        ];
+    }
+}