InterNations / http-mock

Mock HTTP requests on the server side in your PHP unit tests
http://www.internations.org
MIT License
390 stars 62 forks source link

Weird behavior with PHP 7.4, symfony/process and stderr output #53

Closed gabfr closed 4 years ago

gabfr commented 4 years ago

Hello!

After I updated my project to use PHP 7.4, now when I try to run my integration tests I always get a very weird output from the stderr pipe when running HttpMockTrait::tearDownHttpMock();:

HTTP mock server standard error output should be empty
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-''
+'[Fri Feb 21 18:48:56 2020] 127.0.0.1:59798 Accepted
+[Fri Feb 21 18:48:56 2020] 127.0.0.1:59798 Closing
+[Fri Feb 21 18:48:56 2020] 127.0.0.1:59800 Accepted
+[Fri Feb 21 18:48:57 2020] 127.0.0.1:59800 Closing
+[Fri Feb 21 18:48:57 2020] 127.0.0.1:59802 Accepted
+[Fri Feb 21 18:48:57 2020] 127.0.0.1:59802 Closing
+[Fri Feb 21 18:48:57 2020] 127.0.0.1:59804 Accepted
+[Fri Feb 21 18:48:57 2020] 127.0.0.1:59804 Closing
+[Fri Feb 21 18:48:57 2020] 127.0.0.1:59806 Accepted
+[Fri Feb 21 18:48:57 2020] 127.0.0.1:59806 Closing
+[Fri Feb 21 18:48:57 2020] 127.0.0.1:59808 Accepted
+[Fri Feb 21 18:48:57 2020] 127.0.0.1:59808 Closing
+[Fri Feb 21 18:48:57 2020] 127.0.0.1:59810 Accepted
+[Fri Feb 21 18:48:57 2020] 127.0.0.1:59810 Closing
+[Fri Feb 21 18:48:57 2020] 127.0.0.1:59812 Accepted
+[Fri Feb 21 18:48:57 2020] 127.0.0.1:59812 Closing
+'

All my tests are failing because of the same error. If I stop calling the $this->tearDownHttpMock();, then the tests starts passing.

Someone else is experiencing this too? It's happening only after I updated the PHP version. This is some kind of new behavior of Php? Shouldn't this output be directed to the stdout pipe instead of the stderr? I mean, these are not errors, they're just logs in fact.

I'm not even sure if the issue is with internations/http-mock or symfony/process, so, I'm sorry if this issue isn't in the correct place.

lstrojny commented 4 years ago

Could you run php -S localhost:8086 and curl localhost:8086 to see if that is just PHP’s webserver emitting these log messages?

mtils commented 4 years ago

Hello, i have the same problem here. This is the output of php:

[Sun Feb 23 10:18:31 2020] PHP 7.4.2 Development Server (http://localhost:8086) started
[Sun Feb 23 10:18:43 2020] [::1]:55146 Accepted
[Sun Feb 23 10:18:43 2020] [::1]:55146 [404]: (null) / - No such file or directory
[Sun Feb 23 10:18:43 2020] [::1]:55146 Closing
lstrojny commented 4 years ago

Alright. This shows that it is a functionality of the built in web server. We probably need to filter out expected logs in http mock. Anybody feels like looking into a patch?

mtils commented 4 years ago

I currently write a small fix. it look somewhat like this:

    // ...
    public static function assertSuccessFullPhpServerOutput($output, $message='')
    {
        foreach (explode("\n", $output) as $line) {
            if (static::isPhpServerErrorOutputLine($line)) {
                static::fail($message ?: "PHP Web Server logged an error: $line");
            }
        }
    }

    public static function isPhpServerErrorOutputLine($line)
    {
        if (!$line) {
            return false;
        }

        $endsWith = function ($haystack, $needle) {
            return substr($haystack, -strlen($needle)) == $needle;
        };

        return !$endsWith($line, 'Accepted') && !$endsWith($line, 'Closing') && !$endsWith($line, ' started');
    }
    // ...

But I doubt the string comparison stuff is a good solution in general...

lstrojny commented 4 years ago

If we need to go down the route, your code looks good. Does passing -q or —quiet or sth similar change the picture?

mtils commented 4 years ago

I created a pull request with another similiar solution. I just filter the non-error lines out before returning them. I played a little with your unit tests and this worked better (created less diff) then the other approach. The Travis builds show that it works in PHP 7.1-7-3 too but the builds are failing. Somebody should also add PHP 7.4 in the travis file.

gabfr commented 4 years ago

If we need to go down the route, your code looks good. Does passing -q or —quiet or sth similar change the picture?

The -q option did not worked here to suppress those server log statements. From the PHP docs it does layoff only the headers (https://www.php.net/manual/en/features.commandline.options.php).

gabfr commented 4 years ago

I created a pull request with another similiar solution. I just filter the non-error lines out before returning them. I played a little with your unit tests and this worked better (created less diff) then the other approach. The Travis builds show that it works in PHP 7.1-7-3 too but the builds are failing. Somebody should also add PHP 7.4 in the travis file.

I made a PR to a fork version of this repo that we maintain here at Linio and added 7.4 to the travis file. Everything went good.

mtils commented 4 years ago

@gabfr Looks very good. I wanted to clean up my PR but saw your work. I just added one request for change to your own PR. Then I think its perfect and we can close my PR here.

gabfr commented 4 years ago

@mtils do you feel like updating your PR with the same changes of mine? or should I submit another?

mtils commented 4 years ago

@gabfr I think it is better if you create a PR of your changes. You're changes are now well tested. I will close mine then. This is terrific team work ;-)

gabfr commented 4 years ago

Thanks guys for the help into this solution!