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

.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),
+        ];
+    }
+}

src/Observers/index.ts

@@ -2,3 +2,4 @@ export * from './ErrorDialogObserver';
 export * from './OutputChannelObserver';
 export * from './Printers';
 export * from './TestResultObserver';
+export * from './TestRunnerObserverFactory';

src/PHPUnit/PHPUnitXML.ts

@@ -176,9 +176,7 @@ export class PHPUnitXML {
                             .querySelectorAll(type)
                             .map((node) => callback(type, node, parent));
 
-                        if (temp) {
-                            results.push(...temp);
-                        }
+                        results.push(...temp);
                     }
 
                     return results;

src/PHPUnit/TestCollection/TestCollection.ts

@@ -13,6 +13,7 @@ export interface File<T> {
 export class TestCollection {
     private suites = new Map<string, Map<string, TestDefinition[]>>();
     private matcherCache = new Map<string, Map<string, Minimatch>>();
+    private fileIndex = new Map<string, string>();
 
     constructor(private phpUnitXML: PHPUnitXML) {}
 
@@ -55,6 +56,7 @@ export class TestCollection {
             return this;
         }
         files.get(testsuite)?.set(uri.toString(), testDefinitions);
+        this.fileIndex.set(uri.toString(), testsuite);
 
         return this;
     }
@@ -79,18 +81,24 @@ export class TestCollection {
         }
         this.suites.clear();
         this.matcherCache.clear();
+        this.fileIndex.clear();
 
         return this;
     }
 
     findFile(uri: URI): File<TestDefinition> | undefined {
-        for (const file of this.gatherFiles()) {
-            if (uri.toString() === file.uri.toString()) {
-                return file;
-            }
+        const uriStr = uri.toString();
+        const testsuite = this.fileIndex.get(uriStr);
+        if (!testsuite) {
+            return undefined;
+        }
+
+        const tests = this.items().get(testsuite)?.get(uriStr);
+        if (!tests) {
+            return undefined;
         }
 
-        return undefined;
+        return { testsuite, uri, tests };
     }
 
     protected async parseTests(uri: URI, testsuite: string) {
@@ -108,6 +116,7 @@ export class TestCollection {
     }
 
     protected deleteFile(file: File<TestDefinition>) {
+        this.fileIndex.delete(file.uri.toString());
         return this.items().get(file.testsuite)?.delete(file.uri.toString());
     }
 

src/PHPUnit/TestParser/AttributeParser.ts

@@ -29,11 +29,13 @@ export class AttributeParser {
     }
 
     public isTest(method: Method) {
-        return !method.attrGroups
-            ? false
-            : this.parseAttributes(method).some(
-                  (attribute: ParsedAttribute) => attribute.name === 'Test',
-              );
+        if (!method.attrGroups) {
+            return false;
+        }
+
+        return this.parseAttributes(method).some(
+            (attribute: ParsedAttribute) => attribute.name === 'Test',
+        );
     }
 
     private parseAttributes(declaration: Declaration): ParsedAttribute[] {