Skip to content

fix Issue 462 re failing tests on windows and permission denied #463

wants to merge 6 commits into from

4 participants

hans-d commented May 16, 2012

All tests\cases are now passing on Windows with git's core.autocrlf true.

Remains some exceptions (see excerpt below) that are thrown when a readonly file is to be unlinked. Even with a try/catch I couldn't silence this.

Running test(s) in lithium\tests\cases... done.

4356 / 4356 passes
0 fails and 2 exceptions
Exception thrown in -::unlink() on line 1014:
unlink(C:\Users\username\AppData\Local\Temp/tmp/tests\li3_docs.git\objects\pack\pack-744d0770ee39778d4daa376769f06a6b20ffde3e.idx): Permission deniedTrace: lithium\test\Unit::_cleanUp(), line 1014

hans-d added some commits May 15, 2012
@hans-d hans-d fix failing tests/cases/console due to \n\n usage in autocrlf true
\n\n was compared with two hard returns in a string. Using git with autocrlf true (windows) this causes differences.
@hans-d hans-d fix permission denied on unlink
if file to unlink is readonly, it throws a exception (Permission denied) on Windows
solution: remove readonly flag and try again


note: this will still report an exception, but the test will be ok. Try/catch won't solve it
@hans-d hans-d fix failing tests/cases/g11n due to usage with autocrlf true
files are written, independant of the filesystem, with only \n
Testacses might contain extra \r in the compare tests
Union of RAD member

Looks good, except for two things: please issue the pull request against the dev branch so we can CI it. Also, if things need documenting, please add comments in method docblocks, and break tests out into their own methods if necessary. Thanks!

Union of RAD member

You don't need preg_replace() because str_replace() will do the same. The test expectations shouldn't be changed in general but conditionally. Test for the side effects of autocrlf then change expectation. Try to break up tests and use skipIfs as @nateabele suggested.

Also I'm not sure if this behavior will not bite us in the future a couple of times. Seems easy to introduce failing tests etc.

updated with pointers to the skipIf

hans-d added some commits May 16, 2012
@hans-d hans-d Testing for EOL handling in tests
Depending on the platform and git config core.autocrlf setting.
Added _isEolCrLf() utility function to assist in those cases.
@hans-d hans-d Simplify EOL test issues fix.
command/create: re use the already existing <?php ?> replace
g11/catalog/adapter: make EOL replacement conditional
@hans-d hans-d Move inline comments to method documentation 70dd3a8
hans-d commented May 16, 2012

closing this one - will make request against dev

@hans-d hans-d closed this May 16, 2012
@gwoo gwoo commented on the diff May 16, 2012
@@ -1052,6 +1061,26 @@ protected function _hasNetwork($config = array()) {
return !$failed;
+ /**
+ * Provides how end-of-lines (EOL) are represented, either CRLF or LF
+ *
+ * The distinctions is required for some string testing, and
+ * is caused by platform differences between Windows and Unix/Linux.
+ * The git config core.autocrlf also plays a role how EOLs are handled
+ * between the working copy and the repository; simply testing on which
+ * platform the test is run is thus not sufficient.
+ */
+ protected function _Eol() {
Union of RAD member
gwoo added a note May 16, 2012

would the PHP_EOL suffice?

hans-d added a note May 16, 2012

Unfortunately, no. That one does not account for the git settings used. I now derive it from how the heredoc string is present in the file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.