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

fix tests on windows #2053

Merged
merged 6 commits into from Aug 27, 2017
Merged

fix tests on windows #2053

merged 6 commits into from Aug 27, 2017

Conversation

yurii-github
Copy link
Contributor

@yurii-github yurii-github commented Jul 23, 2017

fixes tests on windows OS

@mention-bot
Copy link

@yurii-github, thanks for your PR! By analyzing the history of the files in this pull request, we identified @sarnowski, @splitbrain and @Klap-in to be potential reviewers.

}

$this->file = tempnam($dir, 'css');

$import = '';
do {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed potential infinite loop

@@ -14,17 +14,17 @@ public function setUpFiles($subdir = '') {
mkdir($dir, 0777, true);
}
if (!is_dir($dir)) {
$this->markTestSkipped('Could not create directory.');
throw new Exception('Could not create directory.');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

throw exception instead of skip because this is issue

Copy link
Collaborator

@splitbrain splitbrain left a comment

Choose a reason for hiding this comment

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

I generally like to make our tests run properly on Windows again, though I have to admit I have no way of ensuring it will stay that way.

Some minor changes should be made before merging this.

$this->assertEquals('moep@example.com, foo@example.com', $headers['To']);
} else {
$this->assertEquals('=?UTF-8?B?TcO2cA==?= <moep@example.com>, foo <foo@example.com>', $headers['To']);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think for tests which's task is testing something that does not work/exist in Windows, it would make more sense to just mark the test as skipped. Eg. here the idea is to test the encoding in the name, but since the name is omitted on windows, there is nothing to test that hasn't been tested above already.

@@ -38,10 +38,15 @@ private function csstest($input, $expected_css, $expected_less) {
$this->assertEquals($expected_less, $less);
}

// make relative
private function importPath($path) {
return isWindows() ? preg_replace('#(^.*[\\\\])#','', $path) : preg_replace('#(^.*[/])#','', $path);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not a fan of the tertiary operator here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer to have a proper if-then-else construct here instead of the ... ? ... : ... shortcut.

inc/init.php Outdated
@@ -3,6 +3,20 @@
* Initialize some defaults needed for DokuWiki
*/

if (!function_exists('isWindows')) {
// checks if it is windows OS
Copy link
Collaborator

Choose a reason for hiding this comment

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

proper doc blocks should be used.

inc/init.php Outdated
}

if (!function_exists('w2u')) {
// convert windows path to unix-like on windows OS
Copy link
Collaborator

Choose a reason for hiding this comment

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

proper doc block should be used

inc/init.php Outdated
// convert windows path to unix-like on windows OS
function w2u($filename) {
return isWindows() ? str_replace('\\', '/', $filename) : $filename;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure this is needed/useful outside of tests. PHP does not care for forward or backward slashes when accessing files. So this is only ever needed when comparing files read from the file system with predefines (as done in the tests). So if this only needed for testing it could be moved to the test's utility functions.

inc/init.php Outdated
@@ -3,6 +3,20 @@
* Initialize some defaults needed for DokuWiki
*/

if (!function_exists('isWindows')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why the function_exists check here?

@splitbrain splitbrain merged commit cd08f3e into dokuwiki:master Aug 27, 2017
@splitbrain
Copy link
Collaborator

I merged this and set up automated windows testing at AppVeyor. See https://ci.appveyor.com/project/splitbrain/dokuwiki

@yurii-github yurii-github deleted the master-fix-windows branch August 30, 2017 17:44
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

3 participants