Update http-factory-tests #113
Conversation
| @@ -41,7 +41,7 @@ public function createStream(string $content = ''): StreamInterface | |||
| */ | |||
| public function createStreamFromFile(string $filename, string $mode = 'r'): StreamInterface | |||
| { | |||
| $resource = fopen($filename, $mode); | |||
| $resource = @fopen($filename, $mode); | |||
There was a problem hiding this comment.
Yeah I know I dont like it too, but fopen is logging a warning in some of the tests which leads phpunit to fail the test.
Also in the next lines we are checking the response of fopen where we are throwing an Exception is case we didnt get a resource.
There was a problem hiding this comment.
Alright makes sense. Thanks for the explanation.
Edit:
Actually, is there no other way around this? And could you show what the warnings are? Because they’re not showing up on the CI checks.
There was a problem hiding this comment.
Currently I don't know if we can do something else (more elegenat) here. As we have the PhpFactoryFUnctionOverrides.php we could suppress the warnings just for PHPUnit. Also we are able to disable the transformation of warnings to errors in PHPUnit, either per file, test or in general. (https://stackoverflow.com/a/8412996/3893182)
Until now the warnings are not there because the tests producing the warnings are added with the new version (0.6) of the http-factory-tests library.
The warnings are:
- fopen(/tmp/http_factory_tests_FChC1g): failed to open stream: No such file or directory
- fopen(): Filename cannot be empty
There was a problem hiding this comment.
@danopz Shouldn't we add a file_exists() instead of suppressing the warning?
There was a problem hiding this comment.
@l0gicgate just had a quick look, using file_exists we would still have the problem that a file exists we just don't have access on. For example the file belongs to a different user and there are no permission for our php user.
There was a problem hiding this comment.
@danpopz I'm confused as to why it's trying to open a file that doesn't exist though (/tmp/http_factory_tests_FChC1g). It should exist during the test no? and why are we trying to do fopen() without specifying a filename?
If these tests are written to create failures, then that's just a bad test.
There was a problem hiding this comment.
Sry should have explained a bit better. The problem Im describing is not about the tests but general. Someone could just use it like I described which would then again produce a warning - sure they should take care about their file system.
If you prefer I could still just add the file_exists.
There was a problem hiding this comment.
I'd rather make things as robust as possible with logic rather than silence warnings. They're typically there for a reason and tests should fail in the event that those warnings arise. Let's use file_exists()
There was a problem hiding this comment.
Now I have the problem of handling php://input and some tests with wrong file mode..
|
Closing. #125 was merged. |
Updated the library for our PSR-17 implementation, fixed failing unit tests