Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Mock fopen failure, closes #155 #156

Closed
wants to merge 1 commit into from
Closed

Mock fopen failure, closes #155 #156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 28 additions & 0 deletions src/main/php/org/bovigo/vfs/vfsStreamFile.php
View file
Open in desktop
Expand Up @@ -35,6 +35,10 @@ class vfsStreamFile extends vfsStreamAbstractContent
* @type bool[string]
*/
protected $sharedLock = array();
/**
* @var null|string
*/
private $openError;

/**
* constructor
Expand Down Expand Up @@ -391,4 +395,28 @@ public function hasExclusiveLock($resource = null)

return null !== $this->exclusiveLock;
}

/**
* Setter to set (string w/ content) or remove (null) triggering an error
* on a vfs-stream-wrapped fopen() call.
*
* @param string|null $error string (non-zero length) to raise an error on fopen, null to unset (default)
* @since 1.6.5
* @see https://github.com/mikey179/vfsStream/issues/155
*/
public function setOpenError($error = null)
{
$this->openError = $error;
}

/**
* @see setOpenError
* @return null|string
* @since 1.6.5
* @see https://github.com/mikey179/vfsStream/issues/155
*/
public function getOpenError()
{
return $this->openError;
}
}
14 changes: 11 additions & 3 deletions src/main/php/org/bovigo/vfs/vfsStreamWrapper.php
View file
Open in desktop
Expand Up @@ -278,10 +278,12 @@ protected function resolvePath($path)
*/
public function stream_open($path, $mode, $options, $opened_path)
{
$triggerErrors = ($options & STREAM_REPORT_ERRORS) === STREAM_REPORT_ERRORS;

$extended = ((strstr($mode, '+') !== false) ? (true) : (false));
$mode = str_replace(array('t', 'b', '+'), '', $mode);
if (in_array($mode, array('r', 'w', 'a', 'x', 'c')) === false) {
if (($options & STREAM_REPORT_ERRORS) === STREAM_REPORT_ERRORS) {
if ($triggerErrors) {
trigger_error('Illegal mode ' . $mode . ', use r, w, a, x or c, flavoured with t, b and/or +', E_USER_WARNING);
}

Expand All @@ -293,7 +295,7 @@ public function stream_open($path, $mode, $options, $opened_path)
$this->content = $this->getContentOfType($path, vfsStreamContent::TYPE_FILE);
if (null !== $this->content) {
if (self::WRITE === $mode) {
if (($options & STREAM_REPORT_ERRORS) === STREAM_REPORT_ERRORS) {
if ($triggerErrors) {
trigger_error('File ' . $path . ' already exists, can not open with mode x', E_USER_WARNING);
}

Expand All @@ -313,11 +315,17 @@ public function stream_open($path, $mode, $options, $opened_path)
$this->content->openForAppend();
} else {
if (!$this->content->isReadable(vfsStream::getCurrentUser(), vfsStream::getCurrentGroup())) {
if (($options & STREAM_REPORT_ERRORS) === STREAM_REPORT_ERRORS) {
if ($triggerErrors) {
trigger_error('Permission denied', E_USER_WARNING);
}
return false;
}
if ($error = $this->content->getOpenError()) {
if ($triggerErrors) {
trigger_error($error, E_USER_WARNING);
}
return false;
}
$this->content->open();
}

Expand Down
18 changes: 18 additions & 0 deletions src/test/php/org/bovigo/vfs/vfsStreamWrapperFileTestCase.php
View file
Open in desktop
Expand Up @@ -454,4 +454,22 @@ public function cannotReadFileFromNonReadableDir()
$this->bar->chmod(0);
$this->assertFalse(@file_get_contents($this->baz1URL));
}

/**
* @test
* @see https://github.com/mikey179/vfsStream/issues/155
* @since 1.6.5
*/
public function errorOnOpen()
{
$file = $this->baz1;
$path = $file->url();
$this->assertNotFalse(@fopen($path, 'r'), 'precondition');

$file->setOpenError("fopen() gives error");
$this->assertFalse(@fopen($path, 'r'), 'fopen fails now');

$file->setOpenError(null);
$this->assertNotFalse(@fopen($path, 'r'), 'fopen works again');
}
}
Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

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:

  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. :)