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

[FEATURE] Expect warning / notice #3758

Closed
jasny opened this issue Jul 16, 2019 · 7 comments
Closed

[FEATURE] Expect warning / notice #3758

jasny opened this issue Jul 16, 2019 · 7 comments

Comments

@jasny
Copy link

@jasny jasny commented Jul 16, 2019

Throwing an exception for warnings and notices makes it difficult to test these cases. This could be improved by the addition of expectWarning and expectNotice.

Why?

By default, PHPUnit converts PHP errors, warnings, and notices that are triggered during the execution of a test to an exception. Using these exceptions, you can, for instance, expect a test to trigger a PHP error

In PHP code will continue to run after a warning or notice. Throwing an exception makes it difficult to test if code behaves correctly after the warning is triggered.

When testing that relies on PHP functions that trigger errors like fopen it can sometimes be useful to use error suppression while testing. This allows you to check the return values by suppressing notices that would lead to a PHPUnit PHPUnit\Framework\Error\Notice.

This workaround isn't great. It requires two tests, one for the warning and one for the result. The second test will suppress all warnings and notices. This includes possible issues that happen after the initial warning.

Feature request

Please add the methods expectWarning, expectWarningRegex, expectNotice and expectNoticeRegex. These methods can be called multiple times if multiple warnings and/or notices are expected.

Expected warnings and notices aren't converted to exceptions, but other warnings/notices still are.

<?php
use PHPUnit\Framework\TestCase;

class ErrorSuppressionTest extends TestCase
{
    public function testFileWriting()
    {
        $this->expectWarning("fopen(/is-not-writeable/file): failed to open stream: No such file or directory");

        $writer = new FileWriter;
        $this->assertFalse($writer->write('/is-not-writeable/file', 'stuff'));
    }
}

class FileWriter
{
    public function write($file, $content)
    {
        $file = fopen($file, 'w');

        if ($file == false) {
            return false;
        }

        // ...
    }
}
@Ocramius

This comment has been minimized.

Copy link
Contributor

@Ocramius Ocramius commented Sep 6, 2019

Disagree on this:

  1. warnings/notices are a really bad way to design APIs: use an exception
  2. if you are testing legacy behavior, use ->expectException(\PHPUnit PHPUnit\Framework\Error\Notice::class), which works just fine (I also used it in the past)

Overall, code that raises notices/warnings is faulty, and the fault should be removed, not tested.

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

@sebastianbergmann sebastianbergmann commented Sep 6, 2019

PHPUnit 8.4 has #3775.

@jasny

This comment has been minimized.

Copy link
Author

@jasny jasny commented Sep 6, 2019

@Ocramius The way that PHP uses warning and notices; you're right because those are actually errors. But there are valid reasons to use them. Almost always as runtime warnings;

Examples;

  • Warning: Couldn't write to audit log; hash 'e3b0c44298f...' will not be present
  • Notice: Credits for foo.example.com API running low; only 100 left

@sebastianbergmann This still ends the execution on a warning or notice 😞

Silencing with @ isn't a good workaround, it skips over PHP warnings that happen after your own warning.

@Ocramius

This comment has been minimized.

Copy link
Contributor

@Ocramius Ocramius commented Sep 7, 2019

Discussed it yesterday with @sebastianbergmann, and it is possible to test these edge cases by using a custom error handler that collects warnings/notices, and then assert on what was collected.

@jasny

This comment has been minimized.

Copy link
Author

@jasny jasny commented Sep 8, 2019

@Ocramius Let's switch the argument around. What's the point of the current implementation of expectWarning() etc?

If you expect the code to run and a warning is triggered unexpectedly PHPUnit correctly makes the test fail. If there is an assertion where the code should stop executing if it fails, you'll use an exception and not a warning.

So the only reason to test for a warning in a unit test if when you explicitly wanted to use a warning. The only reason to choose a warning over an not an exception is that you do not want the code break at that point.

So while I agree that warnings and notices should be used sparsely, when they are used you always want to check that the code continues executing correctly and literally never should assume it does so.

PHPUnit already creates a 'custom' error handler. It should support capturing expected warnings and notices correctly or not at all.

TL;DR

Regarding the need to test if the code continues correctly after an expected warning as an "edge case" is not correct. You should either not use/expect warnings at all OR you should always test that the warning doesn't influence the purpose of the function.

@jasny

This comment has been minimized.

Copy link
Author

@jasny jasny commented Sep 8, 2019

@sebastianbergmann Is it for an option for me to create a PR that implements this feature, or will it be rejected regardlessly?

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

@sebastianbergmann sebastianbergmann commented Sep 8, 2019

Such a feature would be rejected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.