withContext(...) feature reuqest

[top-master]

In our project's testRoutes_shouldDenyCustomersAccessToAdminPages() test, we have a for-loop that iterates each and every "admin" prefixed route of our PHP App.

Now sometimes an error is thrown, or the assertion fails, and we would like to know for which route exactly, for that, said for-loop stores the context in lastRoute instance-variable of test-case, and TestListener prefixes the logged message with said context, like:

public function addError(Test $test, \Throwable $t, float $time): void
{
    $this->onThrow($test, $t);
}
public function addFailure(Test $test, AssertionFailedError $error, float $time): void
{
    $this->onThrow($test, $error);
}

private function onThrow(Test $test, \Throwable $error)
{
    if ($test instanceof MyTest) {
        if ($test->lastRoute !== null) {
            self::prefixMessage($error, 'While testing route = ' . $test->lastRoute . ":\n");
        }
    }
}

private static function prefixMessage($obj, $prefix)
{
    try {
        $reflection = new \ReflectionClass($obj);
        $property = $reflection->getProperty('message');
        $property->setAccessible(true);
        $property->setValue($obj, $prefix . $property->getValue($obj));
    } catch (\ReflectionException $e) {
    }
}

---

@sebastianbergmann How can we achive above with the new TestHook API?
Which looks like:

interface AfterTestErrorHook extends TestHook
{
    public function executeAfterTestError(string $test, string $message, float $time): void;
}

interface AfterTestFailureHook extends TestHook
{
    public function executeAfterTestFailure(string $test, string $message, float $time): void;
}

I mean, we could Make the lastRoute a static-variable (instead of instance-variable).
But how can we alter the $message?

Solutions

As you deprecated our side of handling this,
please consider implementing one of below solutions into PHPUnit API.

# 1 You could change your existing API's $message to &$message, I mean, to a reference, then use reflection like above (to update message).

# 2 Or, like some other frameworks, provide withContext(...) method, and the next failure (no matter if error or assert) should log the context before actual message, then clear context.

Also, test-runner should clear context after each test-method (of course).

[sebastianbergmann]

1. The TestHook API is not new and it will be removed. The background is explained here.

2. I do not understand the problem you are trying to solve, therefore I cannot help you.

From the example code you shared, I get the impression that you are coupling your test code too tightly to your production code. For instance, you're testing private implementation details. I get the feeling that you are investing a lot of time and effort into testing infrastructure that would be far better invested into refactoring your production code to be easier to test.

Please understand that this topic is outside the scope of an issue tracker.

[top-master]

@sebastianbergmann

First of all, thanks for soon-removing TestHook thing, was not useful for me at least.

Secondly, let's ignore the well explained "situation and background", as you misunderstand ;-)

• Iterating each and every route is never "tight coupled", but maybe you think we list routes manually?

• No, our test is not even "loose coupled", because the route-list is auto-generated and test will pass even if we have not a signle route (and only fails for access violations).

• If you meant that TestListener is coupled, then because I ask for withContext(...) feature and want to remove TestListener, that does not matter.

Last but not least, please add some-way (feature) to log a custom-message, and that right at top of the next failure's log:

• We can't (even if we wanted) edit the message of exceptions all over the application.
• I don't want to change each assert's message manually.

Please understand that there is a feature-request option in your issue tracker, at least at time of writting, feel free to remove that if you don't like feature requests ;-)

[top-master]

Note that someone already suggested data-provider, and here is my answer:

First of all, that would make the message look something like:

1) Tests\MyTest::testRoutes_shouldDenyCustomersAccessToAdminPages with data set #3 ('/admin/users/edit')
Failed asserting that ...

Instead of:

1) Tests\MyTest::testRoutes_shouldDenyCustomersAccessToAdminPages
While testing route = /admin/users/edit:
Failed asserting that ...

And someone later running the tests would get confused (as we don't write tests for ourselves).

Secondly, storing each and every route in array is not always possible, for example:

• If each route (or data-set) requires some additional data.
• The data-provider would need to pass (i.e. return) those, too.
• Which all go into our logs, making the already confusing data-provider-log even more unreadable.

Last and least, rewritting logic to create array with specific structure, instead of simply iterating would take time.

[remorhaz]

You can use string keys in data providers, so you can receive any text you wish instead of #3 in error message.

rewritting logic to create array with specific structure, instead of simply iterating would take time.

You don't need to create an array: data provider can be a generator, so you can just yield data sets from within iteration cycle.

[top-master]

I already found a solution, which is far more human-readable as well:

#4961 (comment)

No need to hack Data-provider to log pretty messages.

However, Data-provider is enabled through doc-comment, which I never did like,
and once test fails, all that Data-provider offers seems to be logging index and values-provided.