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.

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

Enforce error_reporting level for phpunit suite #117

Closed

Conversation

acoulton
Copy link
Contributor

@acoulton acoulton commented Oct 6, 2015

Many of the unit tests rely on phpunit's error handler throwing exceptions from various trigger_error calls. This error handler only fires for errors matching the system error_reporting configuration.

If the user's local error_reporting is configured too low then the test suite will produce a number of false failures and false positives - expected exceptions will not be thrown and code that is expected to abort will continue execution with the error message silenced. This caught me out when contributing #116

Add a custom bootstrap to ensure that PHP is configured to use E_ALL | E_STRICT for the duration of the test run regardless of the system default setting.

Many of the unit tests rely on phpunit's error handler throwing
exceptions from various trigger_error calls.

If the user's local error_reporting is configured too low then
the test suite will produce a number of false failures and
false positives - expected exceptions will not be thrown and
code that is expected to abort will continue execution with the
error message silenced.

Add a custom bootstrap to ensure that PHP is configured to use
E_ALL | E_STRICT for the duration of the test run regardless of
the system default setting.
@acoulton
Copy link
Contributor Author

acoulton commented Oct 6, 2015

For example, on PHP 5.5.9 running the suite with error_reporting = 0 gives the following failures:

There were 9 failures:

1) org\bovigo\vfs\PermissionsTestCase::touchOnNonWriteableDirectoryTriggersError
Failed asserting that exception of type "PHPUnit_Framework_Error" is thrown.

2) org\bovigo\vfs\vfsStreamWrapperMkDirTestCase::mkDirShouldNotOverwriteExistingDirectoriesAndTriggerE_USER_WARNING
Failed asserting that exception of type "PHPUnit_Framework_Error" is thrown.

3) org\bovigo\vfs\vfsStreamWrapperMkDirTestCase::mkDirShouldNotOverwriteExistingFilesAndTriggerE_USER_WARNING
Failed asserting that exception of type "PHPUnit_Framework_Error" is thrown.

4) org\bovigo\vfs\vfsStreamWrapperMkDirTestCase::unlinkCanNotRemoveEmptyDirectory
Failed asserting that false is true.

/vagrant/vfsstream/src/test/php/org/bovigo/vfs/vfsStreamWrapperDirTestCase.php:428

5) org\bovigo\vfs\vfsStreamWrapperSelectStreamTestCase::selectStream
Failed asserting that exception of type "\PHPUnit_Framework_Error" is thrown.

6) org\bovigo\vfs\vfsStreamWrapperTestCase::renameFileIntoFile
Failed asserting that exception of type "PHPUnit_Framework_ExpectationFailedException" matches expected exception "PHPUnit_Framework_Error". Message was: "Failed asserting that false is true." [...etc]

7) org\bovigo\vfs\vfsStreamWrapperTestCase::renameOnSourceFileNotFound
Failed asserting that exception of type "PHPUnit_Framework_Error" is thrown.

8) org\bovigo\vfs\vfsStreamWrapperTestCase::renameOnDestinationDirectoryFileNotFound
Failed asserting that exception of type "PHPUnit_Framework_Error" is thrown.

9) org\bovigo\vfs\vfsStreamWrapperTestCase::openFileWithoutDirectory
Failed asserting that exception of type "PHPUnit_Framework_Error" is thrown.

FAILURES!
Tests: 385, Assertions: 1327, Failures: 9, Skipped: 5.

I found it wasn't immediately obvious why they were failing (my error_reporting was only 0 due to a misconfiguration in php.ini so it didn't occur at first). This PR ensures the suite behaves as expected regardless of system configuration.

Again, it seems to have reduced coverage although the travis build is green - I'm not certain why.

mikey179 added a commit that referenced this pull request Oct 6, 2015
This fixes the error reporting level so that it is the same independent
of the systems php.ini setting. This fixes the issue raised with #117
but without introducing an additional bootstrap file.
@mikey179
Copy link
Member

mikey179 commented Oct 6, 2015

Thanks for the report, some things only become obvious when other people run the code. I added an additional ini setting in the phpunit.xml.dist file, this saves us from the additional bootstrap file. 😃

@mikey179 mikey179 closed this Oct 6, 2015
@acoulton
Copy link
Contributor Author

acoulton commented Oct 6, 2015

@mikey179 thanks - I'd not thought of that (not an expert on the phpunit.xml options), it's a much better solution. :)

@acoulton acoulton deleted the force-error-reporting-for-unit-tests branch October 6, 2015 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants