Skip to content

Define IS_WINDOWS in the test runner - #4866

Closed
villfa wants to merge 1 commit into
php:masterfrom
villfa:ref-run-tests
Closed

Define IS_WINDOWS in the test runner#4866
villfa wants to merge 1 commit into
php:masterfrom
villfa:ref-run-tests

Conversation

@villfa

Copy link
Copy Markdown
Contributor

Avoid code duplication

Comment threadrun-tests.php
if (array_key_exists('CGI', $section_text) || !empty($section_text['GET']) || !empty($section_text['POST']) || !empty($section_text['GZIP_POST']) || !empty($section_text['DEFLATE_POST']) || !empty($section_text['POST_RAW']) || !empty($section_text['PUT']) || !empty($section_text['COOKIE']) || !empty($section_text['EXPECTHEADERS'])) {
if (isset($php_cgi)) {
$php = $php_cgi . ' -C ';
} else if (!strncasecmp(PHP_OS, "win", 3) && file_exists(dirname($php) . "/php-cgi.exe")) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suppose the usage of strncasecmp() is irrelevant compared to substr() but it is weird seeing it.

Comment threadrun-tests.php
// Parallel testing
global $workers, $workerID;

define('IS_WINDOWS', substr(PHP_OS, 0, 3) == "WIN");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I personally think this is cleaner:

PHP_OS_FAMILY == 'Windows'

The substr() call has always been rather redundant for Windows unless you wanted to differentiate between Windows NT and Windows 9x series.

Either way I'm fine with this change overall

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

PHP_OS_FAMILY is in 7.2, but run-tests is currently compatible with 7.0.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

PHP_OS_FAMILY is in 7.2, but run-tests is currently compatible with 7.0.

As this is targeting master shouldn't this be alright? Or is run-tests used version agnostic?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Context: #3981

We can bump the minimum version requirement, I'm just saying that this is the status quo.

@villfa
villfa deleted the ref-run-tests branch October 28, 2019 12:15
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@villfa@KalleZ@nikic@Girgias@carusogabriel