Mock fopen failure, closes #155

[ktomk]

Feature to test fopen() error cases, to simulate race conditions on
fopen() that is when a readable file becomes unreadable on fopen()
instantly, for example to simulate a file-system failure.

[coveralls]

Coverage decreased (-0.2%) to 94.138% when pulling 272b712 on ktomk:issue-155-mock-fopen-failure into 03c1427 on mikey179:master.

[coveralls]

Coverage decreased (-0.2%) to 94.138% when pulling e92f207 on ktomk:issue-155-mock-fopen-failure into 03c1427 on mikey179:master.

[ktomk]

I don't know of any way to make strea_fopen having options triggering errors. If you know how to provoke that setting, I gladly extend the test-case for better coverage.

/ref https://bugs.php.net/bug.php?id=44124

[mikey179]

Thanks for the PR!

Looking over it I think instead of extending vfsStreamFile it would be better to introduce a new type ErronousFile (just a name idea, not sure if good enough) and put the new behaviour (including the trigger_error() call) inside that new class. Probably this requires to introduce a boolean return value to the open*() methods and let vfsStreamWrapper::stream_open() act accordingly. This would allow to add this feature independent of the flags provided for opening the file - the way you implemented it works only if you don't open the file with flags for appending or truncating it on open (the $mode flag).

A minor (literally ;) thing regarding the @since tags: as this is a new feature this would require a jump to 1.7.0, not just a new bugfix version.

[mikey179]

This should be three different tests, so if one fails we know exactly which one. I know a lot of the existing tests don't always adhere to this rule, but new tests should follow the "one assertion per test" rule.

[ktomk]

I can change that, but I think this is not a correct rule or most often misunderstood. For the test written if it fails, I have left a message on the different expectations so that you exactly know that. The overall test-method asserts the only one thing of that feature, which is more what "one assertion per test" means. These are easy to do btw, extract a private method that starts with assert in it's name and then implement that single one assertion in the private method. That makes this more visible. So the rule has its grounds and is a good one, but when I read such comments I think that programmers sometimes read that too literally and they think there could not be more than an assertion call. And that's not what the rule is for. Hope this leaves some food for thought :) - When refactoring out the new sub-type, this is automatically different again, and I'm totally with you that such a rule is important for a good test-case.

[mikey179]

Well, I should have talked about behaviour instead, I guess... I think there are three different behaviours covered in this test, and each behaviour should have its own test:

1. What happens when no open error is set
2. What happens when an open error is set
3. What happens when an open error is reset

When you look at it like this you automatically end with one assertion per test again. Having more than one assertion per test can be fine, but generally I think it is a potential code smell that the tests could be structured in a better way. These three different behaviours also give a good indication how the according test methods should be named.

On the other hand you are right, this discussion might become moot depending on the other part of the discussion. :)

[mikey179]

I don't know of any way to make strea_fopen having options triggering errors.

Unfortunately I don't know either - that's why all of the trigger_error() parts are not covered...

[ktomk]

I read about your sub-type suggestion and even I think I understand what you mean by that I wonder how this can be used in a test. Is it possible to hint that in the vfs? Can you give some rough pseudo-code about it's usage? I then could try for that as I have this in a repo here already set-up and could change the PR I think relatively easily.

[mikey179]

I think it would look something like this:

$root = vfsStream::setup();
vfsStream::newErronousFile('foo.txt', array('open' => 'error message'))->at($root);

So when you want to let a file operation fail you'd use the specific type and specify which operations should fail. This would also allow to do something like this:

$root = vfsStream::setup();
$file = vfsStream::newErronousFile('foo.txt', array('write' => 'error message'))->at($root);
$fp = fopen($file->url(), 'rb+'); // works
fwrite($fp, 'some data'); // fails

Not sure if this is useful at all, but instead of adding only the possibility to let fopen() fail with the new type we can introduce the possibility to let only a specific action fail, and encapsulate all of this within the new type without cluttering the existing file class. Also, I think this would allow to introduce this feature without having to change the vfsStreamWrapper class at all.

Edit: The new type would extend from the existing vfsStreamFile class, and if no error is defined for an operation it would simply call the parent's class method, and in such behave like a regular file.

[ktomk]

@mikey179: That looks understandable to me. The approach is good to add error-functionality over time while not bugging the main functionality. What is the interface to implement when doing it this way?

[mikey179]

@ktomk: As I'd let the new type extend from vfsStreamFile to be able to fall back to standard behaviour easily there is no immediate need to implement an interface, but one could define it as implements vfsStreamContent anyway. If it is possible to keep all the new logic inside this new class I don't see a reason to introduce a new interface.

[bizurkur]

Closing due to inactivity. The issue is still open and this PR has been referenced there with the comments here as the solution.