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

PHPUnit test suite fails on Windows #1354

Merged
merged 5 commits into from
Jul 25, 2014
Merged

Conversation

sun
Copy link
Contributor

@sun sun commented Jul 25, 2014

PHPUnit's own test suite has various failures on Windows currently. There are two dominant causes:

  1. DIRECTORY_SEPARATOR mismatches in PHPT tests.
  2. Expected stack traces in PHPT tests do not account for an additional stack frame that is caused by PHPUnit_Util_PHP_Windows::process().

Lastly, there's a difference in libxml behavior with regard to PHPUnit_Util_XML::load() on Windows, which causes configuration.xml tests to fail, because relative file paths are not resolved correctly - they're resolved base on the current working directory, instead of the directory in which the configuration file is contained; setting DOMDocument::$documentURI does not help in any way (but is required and MUST be set before loading the XML for XInclude to work).

sun added 4 commits July 25, 2014 21:08
PHPUnit_Util_PHP_Windows writes the process isolation script template into the Windows TEMP directory and loads the script by including the file, which causes an extraneous last stack frame in all test results on Windows.
// @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.

@whatthejeff
Copy link
Contributor

This looks good to me (outside of the line note I left you).

@whatthejeff
Copy link
Contributor

Thanks!

whatthejeff added a commit that referenced this pull request Jul 25, 2014
PHPUnit test suite fails on Windows
@whatthejeff whatthejeff merged commit c3a3544 into sebastianbergmann:4.1 Jul 25, 2014
@sun sun deleted the win-fail branch July 25, 2014 22:09
@sun
Copy link
Contributor Author

sun commented Jul 25, 2014

Thanks a lot! This allows me to properly debug the real/remaining failures of #1352 :)

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.

2 participants