Merge pull request #397 from recca0120/refactor/append-output

refactor: pass location and testId to TestRun.appendOutput

Author: recca0120

packages/extension/__mocks__/vscode.ts

@@ -462,7 +462,16 @@ class FakeEventEmitter<T = void> {
 }
 const EventEmitter = FakeEventEmitter;
 
-class FakeTestMessageStackFrame {}
+class FakeTestMessageStackFrame {
+    label: string;
+    uri?: any;
+    position?: any;
+    constructor(label: string, uri?: any, position?: any) {
+        this.label = label;
+        this.uri = uri;
+        this.position = position;
+    }
+}
 const TestMessageStackFrame = FakeTestMessageStackFrame;
 
 const extensions = {

packages/extension/src/Observers/ObserverFactory.test.ts

@@ -1,5 +1,5 @@
 import { PHPUnitXML, type TestDefinition } from '@vscode-phpunit/phpunit';
-import { beforeEach, describe, expect, it, vi } from 'vitest';
+import { beforeEach, describe, expect, it } from 'vitest';
 import type { OutputChannel, TestItem, TestRun } from 'vscode';
 import type { Configuration } from '../Configuration';
 import type { TestCollection } from '../TestCollection/TestCollection';
@@ -14,23 +14,23 @@ describe('ObserverFactory', () => {
 
     beforeEach(() => {
         const testCollection = {
-            getTestDefinition: vi.fn(),
-            resolveDatasetChild: vi.fn(),
+            getTestDefinition: () => undefined,
+            resolveDatasetChild: () => undefined,
         } as unknown as TestCollection;
         outputChannel = {
-            append: vi.fn(),
-            appendLine: vi.fn(),
-            clear: vi.fn(),
-            show: vi.fn(),
+            append: () => {},
+            appendLine: () => {},
+            clear: () => {},
+            show: () => {},
         } as unknown as OutputChannel;
-        const configuration = { get: vi.fn() } as unknown as Configuration;
+        const configuration = { get: () => undefined } as unknown as Configuration;
         const phpUnitXML = new PHPUnitXML();
         factory = new ObserverFactory(testCollection, outputChannel, configuration, phpUnitXML);
     });
 
     it('should create observers including DatasetObserver, TestResultObserver, and PrinterObserver', () => {
         const queue = new Map<TestDefinition, TestItem>();
-        const testRun = { enqueued: vi.fn() } as unknown as TestRun;
+        const testRun = { enqueued: () => {} } as unknown as TestRun;
         const observers = factory.create(queue, testRun);
 
         expect(observers.length).toBe(5);
@@ -41,7 +41,7 @@ describe('ObserverFactory', () => {
 
     it('should create new instances for each call', () => {
         const queue = new Map<TestDefinition, TestItem>();
-        const testRun = { enqueued: vi.fn() } as unknown as TestRun;
+        const testRun = { enqueued: () => {} } as unknown as TestRun;
         const observers1 = factory.create(queue, testRun);
         const observers2 = factory.create(queue, testRun);
 

packages/extension/src/Observers/ObserverFactory.ts

@@ -40,7 +40,10 @@ export class ObserverFactory {
             new DatasetObserver(this.testCollection, testItemById),
             new TestResultObserver(testItemById, queue, testRun),
             new RawOutputObserver(this.outputChannel, this.configuration),
-            new PrinterObserver(new TestRunWriter(testRun), new Printer(this.phpUnitXML, format)),
+            new PrinterObserver(
+                new TestRunWriter(testRun, testItemById),
+                new Printer(this.phpUnitXML, format),
+            ),
             new ErrorDialogObserver(this.configuration),
         ];
     }

packages/extension/src/Observers/PrinterObserver.test.ts

@@ -1,12 +1,14 @@
 import {
     Configuration,
+    type OutputWriter,
     type Path,
     PathReplacer,
     PHPUnitXML,
     PRESET_PROGRESS,
     Printer,
     ProcessBuilder,
     semverLt,
+    TeamcityEvent,
     TestRunner,
 } from '@vscode-phpunit/phpunit';
 import { detectPhpUnitStubs, phpUnitProject } from '@vscode-phpunit/phpunit/testing';
@@ -20,16 +22,22 @@ const writers = [
         writerName: 'OutputChannelWriter',
         createWriter: () => {
             const spy = vi.fn();
-            const outputChannel = { append: spy, appendLine: vi.fn() };
+            const outputChannel = { append: spy, appendLine: () => {} };
             return { writer: new OutputChannelWriter(outputChannel), spy };
         },
     },
     {
         writerName: 'TestRunWriter',
         createWriter: () => {
+            const testRun = { appendOutput: vi.fn() } as unknown as TestRun;
+            const writer = new TestRunWriter(testRun, new Map());
             const spy = vi.fn();
-            const testRun = { appendOutput: spy } as unknown as TestRun;
-            return { writer: new TestRunWriter(testRun), spy };
+            const originalAppend = writer.append.bind(writer);
+            writer.append = (text: string, ...args: unknown[]) => {
+                spy(text);
+                originalAppend(text, ...(args as []));
+            };
+            return { writer, spy };
         },
     },
 ];
@@ -187,3 +195,96 @@ describe.each(writers)('PrinterObserver with $writerName', ({ createWriter }) =>
         });
     });
 });
+
+describe('PrinterObserver passes location and testId to writer', () => {
+    let appendSpy: ReturnType<typeof vi.fn>;
+    let observer: PrinterObserver;
+
+    beforeEach(() => {
+        appendSpy = vi.fn();
+        const writer: OutputWriter = {
+            append: appendSpy as OutputWriter['append'],
+            appendLine: () => {},
+        };
+        observer = new PrinterObserver(writer, new Printer(new PHPUnitXML(), PRESET_PROGRESS));
+    });
+
+    function startTest(name: string, id: string) {
+        observer.testStarted({
+            event: TeamcityEvent.testStarted,
+            name,
+            locationHint: '',
+            flowId: 1,
+            id,
+            file: '/app/tests/MyTest.php',
+        });
+        appendSpy.mockClear();
+    }
+
+    it('testFinished passes file line 1 as location', () => {
+        startTest('test_passed', 'App\\Tests\\MyTest::test_passed');
+
+        observer.testFinished({
+            event: TeamcityEvent.testFinished,
+            name: 'test_passed',
+            locationHint: '',
+            flowId: 1,
+            id: 'App\\Tests\\MyTest::test_passed',
+            file: '/app/tests/MyTest.php',
+            duration: 5,
+        });
+
+        expect(appendSpy).toHaveBeenCalledWith(
+            expect.any(String),
+            { file: '/app/tests/MyTest.php', line: 1 },
+            'App\\Tests\\MyTest::test_passed',
+        );
+    });
+
+    it('testFailed passes detail location when available', () => {
+        startTest('test_fail', 'App\\Tests\\MyTest::test_fail');
+
+        observer.testFailed({
+            event: TeamcityEvent.testFailed,
+            name: 'test_fail',
+            locationHint: '',
+            flowId: 1,
+            id: 'App\\Tests\\MyTest::test_fail',
+            file: '/app/tests/MyTest.php',
+            message: 'Failed asserting that false is true.',
+            details: [
+                { file: 'vendor/phpunit/phpunit/src/Framework/Assert.php', line: 198 },
+                { file: '/app/tests/MyTest.php', line: 27 },
+            ],
+            duration: 0,
+        });
+
+        expect(appendSpy).toHaveBeenCalledWith(
+            expect.any(String),
+            { file: '/app/tests/MyTest.php', line: 27 },
+            'App\\Tests\\MyTest::test_fail',
+        );
+    });
+
+    it('testIgnored passes file line 1 as location', () => {
+        startTest('test_skipped', 'App\\Tests\\MyTest::test_skipped');
+
+        observer.testIgnored({
+            event: TeamcityEvent.testIgnored,
+            name: 'test_skipped',
+            locationHint: '',
+            flowId: 1,
+            id: 'App\\Tests\\MyTest::test_skipped',
+            file: '/app/tests/MyTest.php',
+            message: 'The MySQLi extension is not available.',
+            details: [],
+            duration: 0,
+        });
+
+        expect(appendSpy).toHaveBeenCalledWith(
+            expect.any(String),
+            { file: '/app/tests/MyTest.php', line: 1 },
+            'App\\Tests\\MyTest::test_skipped',
+        );
+    });
+});

packages/extension/src/Observers/PrinterObserver.ts

@@ -1,4 +1,5 @@
 import type {
+    OutputLocation,
     OutputWriter,
     Printer,
     TestConfiguration,
@@ -8,6 +9,7 @@ import type {
     TestIgnored,
     TestProcesses,
     TestResult,
+    TestResultIdentify,
     TestResultSummary,
     TestRunnerObserver,
     TestRuntime,
@@ -52,11 +54,11 @@ export class PrinterObserver implements TestRunnerObserver {
     }
 
     testStarted(result: TestStarted): void {
-        this.append(this.printer.testStarted(result));
+        this.append(this.printer.testStarted(result), this.toLocation(result), result.id);
     }
 
     testFinished(result: TestFinished): void {
-        this.append(this.printer.testFinished(result));
+        this.append(this.printer.testFinished(result), this.toLocation(result), result.id);
         this.flushOutput(result);
     }
 
@@ -65,7 +67,7 @@ export class PrinterObserver implements TestRunnerObserver {
     }
 
     testIgnored(result: TestIgnored): void {
-        this.append(this.printer.testIgnored(result));
+        this.append(this.printer.testIgnored(result), this.toLocation(result), result.id);
         this.flushOutput(result);
     }
 
@@ -88,14 +90,39 @@ export class PrinterObserver implements TestRunnerObserver {
 
     private flushOutput(result?: TestResult): void {
         const output = this.printer.flushOutput(result);
-        if (output) {
+        if (!output) {
+            return;
+        }
+
+        if (!result || !this.isIdentifiable(result)) {
             this.append(output);
+            return;
+        }
+
+        this.append(output, this.toLocation(result), result.id);
+    }
+
+    private isIdentifiable(
+        result: TestResult,
+    ): result is TestResult & TestResultIdentify & { details?: TestFailed['details'] } {
+        return 'id' in result && 'file' in result;
+    }
+
+    private toLocation(
+        result: TestResultIdentify & { details?: TestFailed['details'] },
+    ): OutputLocation | undefined {
+        if (!result.file) {
+            return undefined;
         }
+
+        const detail = result.details?.find(({ file }) => file === result.file);
+
+        return { file: result.file, line: detail?.line ?? 1 };
     }
 
-    private append(text: string | undefined) {
+    private append(text: string | undefined, location?: OutputLocation, testId?: string) {
         if (text !== undefined) {
-            this.writer.append(text);
+            this.writer.append(text, location, testId);
         }
     }
 }

packages/extension/src/Observers/TestResultObserver.test.ts

@@ -1,4 +1,9 @@
-import type { TeamcityEvent, TestDefinition, TestFailed } from '@vscode-phpunit/phpunit';
+import type {
+    TeamcityEvent,
+    TestDefinition,
+    TestFailed,
+    TestFinished,
+} from '@vscode-phpunit/phpunit';
 import { beforeEach, describe, expect, it, vi } from 'vitest';
 import {
     type TestController,
@@ -122,6 +127,68 @@ describe('TestResultObserver', () => {
         expect(testRun.started).toHaveBeenCalledWith(datasetItem);
     });
 
+    it('should pass duration to testRun.passed', () => {
+        observer.testFinished({
+            event: 'testFinished' as unknown as TeamcityEvent,
+            id: 'Tests\\ExampleTest::test_example',
+            flowId: 1,
+            name: 'test_example',
+            file: '/project/tests/ExampleTest.php',
+            locationHint: '',
+            duration: 42,
+        } as TestFinished);
+
+        expect(testRun.passed).toHaveBeenCalledWith(testItem, 42);
+    });
+
+    it('should use 0-based line in message location', () => {
+        observer.testFailed(
+            createTestFailed({
+                details: [
+                    { file: '/project/tests/ExampleTest.php', line: 10 },
+                    { file: '/project/tests/ExampleTest.php', line: 20 },
+                ],
+            }),
+        );
+
+        const failedCall = (testRun.failed as ReturnType<typeof vi.fn>).mock.calls[0];
+        const message = failedCall[1];
+
+        expect(message.location.range.start.line).toBe(9);
+    });
+
+    it('should use 0-based line in stackTrace position', () => {
+        observer.testFailed(
+            createTestFailed({
+                details: [
+                    { file: '/project/tests/ExampleTest.php', line: 10 },
+                    { file: '/project/tests/ExampleTest.php', line: 20 },
+                ],
+            }),
+        );
+
+        const failedCall = (testRun.failed as ReturnType<typeof vi.fn>).mock.calls[0];
+        const message = failedCall[1];
+
+        expect(message.stackTrace).toHaveLength(1);
+        expect(message.stackTrace[0].position.line).toBe(19);
+    });
+
+    it('should not set location when result.file is undefined', () => {
+        observer.testFailed(
+            createTestFailed({
+                file: undefined as unknown as string,
+                details: [{ file: '/project/tests/ExampleTest.php', line: 10 }],
+            }),
+        );
+
+        const failedCall = (testRun.failed as ReturnType<typeof vi.fn>).mock.calls[0];
+        const message = failedCall[1];
+
+        expect(message.location).toBeUndefined();
+        expect(message.stackTrace).toBeUndefined();
+    });
+
     it('should not use TestMessage.diff when expected/actual are missing', () => {
         const diffSpy = vi.spyOn(TestMessage, 'diff');