The Travis setup should be fixed to avoid leaking Varnish error messages in the test output

[stof]

This output should be catched. It may be displayed after the build if it makes sense, but it should not come in the middle of the PHPUnit output as it makes it unreadable

[dbu]

we could redirect the error output to /dev/null, but that will hurt us when we break something.

or we could try to run varnish with sudo - it would work on travis i think, but maybe not everywhere.

[stof]

is it a process launched from PHP or from Travis ?

[dbu]

its launched inside the test, so that the tests can decide what vcl to use:

https://github.com/FriendsOfSymfony/FOSHttpCache/blob/master/tests/VarnishTestCase.php#L155-L164

[stof]

the issue is that exec does not catch STERR but forwards it to the PHP error output.

I suggest using the Process component to launch the command instead. This way, the error output will be catched as well.
And we can make the test throw an exception with the output in it (the Process component provides an exception for this) in case of failed process (like for the case of an invalid VCL file instead of waiting for the test to fail because Varnish is not started), while discarding it if it is only a warning (like for the Not running as root, no priv-sep message)

[ddeboer]

@stof But we store the PID in a file to make sure Varnish is stopped when starting the tests, even if it was still running from a previous (terminated) test run. As far as I can see, the Process component does not have the ability to store the PID externally and/or construct processes from a PID value (which we would then continue to store ourselves).

[stof]

I don't understand what you mean here. I suggest replacing exec by the usage of the Process component, not changing the subprocess being run

[dbu]

@ddeboer there is a Process::getPid method, is that not enough?

[stof]

AFAICT, the code linked by @ddeboer does not handle the PID in PHP. exec does not have more pID handling than the Process component (and probably less actually). The PID is not stored by the PHP code. It is stored by Varnish

[ddeboer]

I mean this: in our VarnishTestCase’s setUp(), Varnish is reset. This means: stop and then start Varnish. Within one and the same test run, this will work with Process and its getPid() method.

However, when a test run is abnormally terminated due to an exception or the user pressing Ctrl+C, Varnish will still be running. If phpunit is then run again, the new test process no longer knows the Varnish PID. That’s why we store that PID in a file so we can track it between test runs. Unfortunately, we cannot construct a Process from a previously known PID, so as far as I can see the Process component is insufficient here. Or am I missing something?

[dbu]

we could use the process component, get the pid from it and store that in a file. and delete the file upon successful termination of the process. if the file is still there, we try to kill the running varnish before starting a new process.

[ddeboer]

Okay, but then we kill the process in two ways: Process::stop() alongside our current kill -9 plus unlink.

[dbu]

i would be ok with that, kill is really a last resort to recover from an error situation.