-
Notifications
You must be signed in to change notification settings - Fork 102
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
Mock fopen failure, closes #155 #156
Conversation
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.
272b712
to
e92f207
Compare
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. |
Thanks for the PR! Looking over it I think instead of extending A minor (literally ;) thing regarding the |
|
||
$file->setOpenError(null); | ||
$this->assertNotFalse(@fopen($path, 'r'), 'fopen works again'); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
- What happens when no open error is set
- What happens when an open error is set
- 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. :)
Unfortunately I don't know either - that's why all of the |
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. |
I think it would look something like this:
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:
Not sure if this is useful at all, but instead of adding only the possibility to let Edit: The new type would extend from the existing |
@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? |
@ktomk: As I'd let the new type extend from |
Closing due to inactivity. The issue is still open and this PR has been referenced there with the comments here as the solution. |
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.