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

PHPUnit test suite fails on Windows #1354

Merged
merged 5 commits into from
Jul 25, 2014
Merged

PHPUnit test suite fails on Windows #1354

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c1bf461
Fixed XML::load() does not report libxml errors on Windows.
sun Jul 25, 2014
b93090b
Fixed XML::load() fails to load relative XInclude paths on Windows.
sun Jul 25, 2014
de80d07
Exclude system temp directory in stack traces on Windows.
sun Jul 25, 2014
6c1e0cb
Fixed PHPT file path expectations fail on Windows.
sun Jul 25, 2014
59a3263
Include process isolation file prefix to avoid false-positive matches.
sun Jul 25, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/Util/Blacklist.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,13 @@ private function initialize()

self::$directories[] = $directory;
}

// Process isolation workaround on Windows.
// @see PHPUnit_Util_PHP::factory()
// @see PHPUnit_Util_PHP_Windows::process()
if (DIRECTORY_SEPARATOR === '\\') {
self::$directories[] = sys_get_temp_dir();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be the only debatable change.

Technically, it not only hides the process isolation script from stack traces, but any PHP code that happens to be executed through a temporary file in the temporary directory.

However, even on the assumption that such an edge case exists in the wild in the first place, then additional stack frames like the following are not exactly helpful anyway:

C:\Users\sun\AppData\Local\Temp\PHP4B92.tmp:323
C:\Users\sun\AppData\Local\Temp\PHP4B92.tmp:444

That's why I added the system's temporary directory to the blacklist instead.

Before doing this change, I considered two alternatives:

  1. Changing PHPUnit_Util_Filter to remove the last stack frames, if their file paths point to the temp directory.
  2. Adjusting all PHPT tests to include an %A where stack traces are asserted in the output, so as to account for the two additional frames on Windows.

(2) would be cumbersome to maintain + tests would be guaranteed to break on Windows in the future again. (1) sounds nice, but requires relatively complex code to introspect each stack trace.

Therefore, I went with this KISS solution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like you could also add the following to PHPUnit_Util_Blacklist::isBlacklisted():

if (DIRECTORY_SEPARATOR === '\\' && 
    strpos($file, sys_get_temp_dir() . '\\PHPUnit') === 0) {
    return true;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strpos($file, sys_get_temp_dir() . '\\PHPUnit') === 0

To quote the PHP manual:

Note: Windows uses only the first three characters of prefix.

The temporary file names are still compatible with filesystems that only support 8.3 filenames.

We can minimally adjust it and append 'PHP' to the blacklisted pathname (which are checked via strpos() anyway), although the likelihood of a prefix-clash would be relatively high, because 'PHP' is a fairly generic prefix for PHP software. 😉

But yeah, let me do that, so as to do the maximum to avoid false-positives.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hard to say what the likelihood of a clash would be without some sort of empirical data. If we really believe it's an issue, we could always add an interface for supplying the exact filenames to blacklist.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've included the prefix now.

}
}
}
26 changes: 18 additions & 8 deletions src/Util/XML.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,36 +135,46 @@ public static function load($actual, $isHtml = false, $filename = '', $xinclude
return $actual;
}

// Required for XInclude on Windows.
if ($xinclude) {
$cwd = getcwd();
chdir(dirname($filename));
}

$document = new DOMDocument;

$internal = libxml_use_internal_errors(true);
$message = '';
$reporting = error_reporting(0);

if ('' !== $filename) {
// Necessary for xinclude
$document->documentURI = $filename;
}

if ($isHtml) {
$loaded = $document->loadHTML($actual);
} else {
$loaded = $document->loadXML($actual);
}

if ('' !== $filename) {
// Necessary for xinclude
$document->documentURI = $filename;
}

if (!$isHtml && $xinclude) {
$document->xinclude();
}

foreach (libxml_get_errors() as $error) {
$message .= $error->message;
$message .= "\n" . $error->message;
}

libxml_use_internal_errors($internal);
error_reporting($reporting);

if ($loaded === false) {
if ($filename != '') {
if ($xinclude) {
chdir($cwd);
}

if ($loaded === false || $message !== '') {
if ($filename !== '') {
throw new PHPUnit_Framework_Exception(
sprintf(
'Could not load "%s".%s',
Expand Down
6 changes: 3 additions & 3 deletions tests/Regression/578.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,17 @@ There were 3 errors:
1) Issue578Test::testNoticesDoublePrintStackTrace
Invalid error type specified

%s/Issue578Test.php:%i
%sIssue578Test.php:%i

2) Issue578Test::testWarningsDoublePrintStackTrace
Invalid error type specified

%s/Issue578Test.php:%i
%sIssue578Test.php:%i

3) Issue578Test::testUnexpectedExceptionsPrintsCorrectly
Exception: Double printed exception

%s/Issue578Test.php:%i
%sIssue578Test.php:%i

FAILURES!
Tests: 3, Assertions: 0, Errors: 3.
2 changes: 1 addition & 1 deletion tests/Regression/GitHub/74.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ There was 1 error:
1) Issue74Test::testCreateAndThrowNewExceptionInProcessIsolation
NewException: Testing GH-74

%s/tests/Regression/GitHub/74/Issue74Test.php:7
%sIssue74Test.php:7

FAILURES!
Tests: 1, Assertions: 0, Errors: 1.
2 changes: 1 addition & 1 deletion tests/TextUI/custom-printer-debug.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ PHPUnit_TextUI_Command::main();
--EXPECTF--
PHPUnit %s by Sebastian Bergmann.

Configuration read from %s/configuration.custom-printer.xml
Configuration read from %sconfiguration.custom-printer.xml


Starting test 'BankAccountTest::testBalanceIsInitiallyZero'.
Expand Down
2 changes: 1 addition & 1 deletion tests/TextUI/custom-printer-verbose.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ PHPUnit_TextUI_Command::main();
--EXPECTF--
PHPUnit %s by Sebastian Bergmann.

Configuration read from %s/configuration.custom-printer.xml
Configuration read from %sconfiguration.custom-printer.xml

I

Expand Down
2 changes: 1 addition & 1 deletion tests/TextUI/dataprovider-log-xml-isolation.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ PHPUnit %s by Sebastian Bergmann.

..F.<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
<testsuite name="DataProviderTest" file="%s/DataProviderTest.php" tests="4" assertions="4" failures="1" errors="0" time="%f">
<testsuite name="DataProviderTest" file="%sDataProviderTest.php" tests="4" assertions="4" failures="1" errors="0" time="%f">
<testsuite name="DataProviderTest::testAdd" tests="4" assertions="4" failures="1" errors="0" time="%f">
<testcase name="testAdd with data set #0" assertions="1" time="%f"/>
<testcase name="testAdd with data set #1" assertions="1" time="%f"/>
Expand Down
2 changes: 1 addition & 1 deletion tests/TextUI/dataprovider-log-xml.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ PHPUnit %s by Sebastian Bergmann.

..F.<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
<testsuite name="DataProviderTest" file="%s/DataProviderTest.php" tests="4" assertions="4" failures="1" errors="0" time="%f">
<testsuite name="DataProviderTest" file="%sDataProviderTest.php" tests="4" assertions="4" failures="1" errors="0" time="%f">
<testsuite name="DataProviderTest::testAdd" tests="4" assertions="4" failures="1" errors="0" time="%f">
<testcase name="testAdd with data set #0" assertions="1" time="%f"/>
<testcase name="testAdd with data set #1" assertions="1" time="%f"/>
Expand Down