Skip to content
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

Wrong backtrace line is reported #5574

Closed
mvorisek opened this issue Nov 24, 2023 · 4 comments · Fixed by #5628
Closed

Wrong backtrace line is reported #5574

mvorisek opened this issue Nov 24, 2023 · 4 comments · Fixed by #5628
Labels
feature/test-runner CLI test runner type/bug Something is broken version/10 Something affects PHPUnit 10

Comments

@mvorisek
Copy link
Contributor

mvorisek commented Nov 24, 2023

Q A
PHPUnit version 10.4.2
PHP version 8.2.12
Installation Method Composer

Summary

Below is minimal repro test with two exceptions created on a separate line. For some reasons, the same line for both exceptions is reported, but the decisive is where the exception was created.

native php dump: https://3v4l.org/EIXk2 (notice line 3 and 4 is reported)

How to reproduce

<?php

declare(strict_types=1);

namespace Repro;

use PHPUnit\Event\Code\ThrowableBuilder;
use PHPUnit\Framework\TestCase;

class ReproTest extends TestCase
{
    public function test(): void
    {
        $line = __LINE__;
        $innerException = new \Error('Inner Exception');
        $exception = new \Exception('My exception', 0, $innerException);

        $phpunitThrowable = ThrowableBuilder::from($exception);
        $phpunitThrowableStr = str_replace(\PHP_EOL, "\n", $phpunitThrowable->asString());

        self::assertSame(
            str_replace('[lineMain]', (string) ($line + 2), str_replace('[lineInner]', (string) ($line + 1), <<<'EOF'
                Exception: My exception

                self.php:[lineMain]

                Caused by
                Error: Inner Exception

                self.php:[lineInner]
                EOF)) . "\n",
            str_replace(__FILE__, 'self.php', $phpunitThrowableStr)
        );
    }
}

Current behavior

1) Atk4\Core\Tests\ReproTest::test
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
 'Exception: My exception

-self.php:16
+self.php:15

 Caused by
 Error: Inner Exception

Expected behavior

no diff

@mvorisek mvorisek added type/bug Something is broken version/10 Something affects PHPUnit 10 version/9 Something affects PHPUnit 9 labels Nov 24, 2023
@sebastianbergmann
Copy link
Owner

ThrowableBuilder is marked @internal and not covered by the backward compatibility promise for PHPUnit. It does not even exist in PHPUnit 9, so if there is a bug here then it would only affect PHPUnit 10.

Please provide a minimal, self-contained, reproducing test case that shows the problem you are reporting without using PHPUnit internals directly.

Without such a minimal, self-contained, reproducing test case I will not be able to investigate this issue.

@sebastianbergmann sebastianbergmann removed the version/9 Something affects PHPUnit 9 label Nov 27, 2023
@mvorisek
Copy link
Contributor Author

Thank you for your reply. Here is minimal repro:

    public function test(): void
    {
        $innerException = new \Error('Inner Exception');
        $exception = new \Exception('My exception', 0, $innerException);

        throw $exception;
    }

PHPUnit 9.6 output (correct):

There was 1 error:

1) xxxTest::test
Exception: My exception

xxxTest.php:19

Caused by
Error: Inner Exception

xxxTest.php:18

ERRORS!
Tests: xxx, Assertions: xxx, Errors: 1.

PHPUnit 10.4 output (incorrect):

There was 1 error:

1) xxxTest::test
Exception: My exception

xxxTest.php:18

Caused by
Error: Inner Exception

xxxTest.php:18

ERRORS!
Tests: xxx, Assertions: xxx, Errors: 1.

Notice PHPUnit 9.6 reports line 19 and 18, but PHPUnit 10.4 reports (wrongly) 18 and 18.

@sebastianbergmann sebastianbergmann added the feature/test-runner CLI test runner label Nov 27, 2023
@sebastianbergmann
Copy link
Owner

Complete reproducing test case based on the one shown in #5574 (comment)

<?php declare(strict_types=1);
use PHPUnit\Framework\TestCase;

final class Issue5574Test extends TestCase
{
    public function testOne(): void
    {
        $innerException = new \Error('Inner Exception');
        $outerException = new \Exception('My exception', 0, $innerException);

        throw $outerException;
    }
}
PHPUnit 10.5-gd128321206 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.0

E                                                                   1 / 1 (100%)

Time: 00:00.016, Memory: 4.00 MB

There was 1 error:

1) Issue5574Test::testOne
Exception: My exception

/home/sb/Issue5574Test.php:8

Caused by
Error: Inner Exception

/home/sb/Issue5574Test.php:8

ERRORS!
Tests: 1, Assertions: 0, Errors: 1.

@samlitowitz
Copy link
Contributor

samlitowitz commented Dec 27, 2023

I believe the root cause is in PHPUnit\Util\Filter::getFilteredStacktrace in the else block on line 44 when called by PHPUnit\Event\Code\ThrowableBuilder::from. The wrapping \Throwable, $outerException in this case, is replaced by the wrapped \Throwable, $innerException in this caseprior to building the stack trace resulting in a duplicate of the wrapped\Throwablestack trace. However thePHPUnit\Event\Code\ThrowableBuilder::fromfunction [also unwraps](https://github.com/sebastianbergmann/phpunit/blob/main/src/Event/Value/ThrowableBuilder.php#L31) the\Throwableand callsPHPUnit\Util\Filter::getFilteredStacktraceon it every time, this means each wrapped\Throwable` will have the stack trace output duplicated.

Given that PHPUnit\Util\Filter::getFilteredStacktrace is called from a number of contexts I don't think we want to change its default behavior. However I do not believe we are able to make a copy of the \Throwable $t argument in PHPUnit\Event\Code\ThrowableBuilder::from.

I see at least two options.

One is create a separate function which replicates the behavior in PHPUnit\Util\Filter::getFilteredStacktrace without the the unwrapping behavior. In my mind the duplication of implementation creates an unecessary risk that behavior is implemented in one function and not the other.

The other option I see is to add a parameter to PHPUnit\Util\Filter::getFilteredStacktrace which controls the unwrapping behavior defaulting to a value of true to retain the current behavior but set to false should address this bug. I've submitted a PR with this solution #5628

Edit: This may be incorrect, I will update if/as it changes. This is correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/test-runner CLI test runner type/bug Something is broken version/10 Something affects PHPUnit 10
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants