- 
                Notifications
    You must be signed in to change notification settings 
- Fork 27
Pass tests on Windows; #2
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
Conversation
Skipped testUnwritableDirInConstructor(), testUnwritableDir() on Windows;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This complete my review. Please respond to the comments so that we can merge the PR 😄
| * @param $filename | ||
| * @return string | ||
| */ | ||
| private function readFileContent($filename) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think renaming this to readFileContentWithoutWhitespaces makes more sense
| return false; | ||
| } | ||
|  | ||
| if (strcmp($expected, $generated) !== 0) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should assert that one string is not empty then that the 2 strings are equal instead of returning values. The method should call an assertion internally rather than returning a value that we would call an assertion against later.
|  | ||
| $expected = $this->readFileContent($expectedFile); | ||
|  | ||
| if (empty($expected)) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should remove this IMO
|  | ||
| $generated = $this->readFileContent($actualFile); | ||
|  | ||
| if (empty($generated)) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here too
| $this->codeFile->writeFile(); | ||
|  | ||
| $this->assertFileEquals(static::getExpectedFilesDir() . "/$this->fileName.php", $this->codeFile->getWritePath()); | ||
| $this->assertTrue( | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we should just call the assertion method. Not call an assertion inside an assertion.
| $this->assertFileEquals( | ||
| static::getExpectedFilesDir() . "/$objectName.php", | ||
| static::getGeneratedFilesDir() . "/$objectName.php" | ||
| $this->assertTrue( | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same in all instances that have the same issue, we should remove the assertTrue
| $this->queryObject = new SimpleQueryObject('simples'); | ||
| } | ||
|  | ||
| private function replaceWhitespace(string $string) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if you replace the method with an assertion, something like assertStringsEqualWithoutWhitespaces and apply that assertion to all assertions in this class.
The problem is coming from php-graphql-client
FieldTrait.php constructSelectionSet()where we use a\ncharacter to build the query.I fixed it and other Windows specified problems in the tests for php-graphql-oqm.
A new line on Windows is
\r\n, linux\ntestUnwritableDirInConstructor(), testUnwritableDir() - we need to skip these tests, because windows don't have chmod().
DIRECTORY_SEPARATOR - depending on a platform
/or\The test results on win7 x64 PHP 7.3.9 MINGW64
vendor/phpunit/phpunit/phpunit tests/ --whitelist src/ --coverage-clover build/coverage/xml
Tests with this PR on win7 x64 PHP 7.3.9 MINGW64