-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
Added copyEmptyDirectories option to FileHelper #14051
Conversation
// clean up assets directory | ||
$handle = opendir($dir = Yii::getAlias('@testAssetsPath')); | ||
if ($handle === false) { | ||
throw new \Exception("Unable to open directory: $dir"); |
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.
RuntimeException?
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.
not sure, what do you mean? this is just to fail tests in case of permission issues.
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.
OK.
|
||
foreach ($bundle->js as $filename) { | ||
$publishedFile = $bundle->basePath . DIRECTORY_SEPARATOR . $filename; | ||
$this->assertTrue(unlink($publishedFile)); |
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 looks like you are testing the 'unlink' function
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.
It's testing for file existence.
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.
is it? Or is it about checking whether the file can be unlinked (due to certain file permissions?).
Would be helpful to add an $error message?
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.
it is cleanup and test in one step. unlink returns true if it was deleted, i.e. it existed, but it will also be removed as it is not needed anymore. That test is copied from similar tests in the same file so I would keep it as is.
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.
don't you want tests to be clear and simple?
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.
Agree with @dynasource. It's better to do it in two steps.
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.
adjusted tests.
$publishedFile = $bundle->basePath . DIRECTORY_SEPARATOR . $filename; | ||
$this->assertTrue(unlink($publishedFile)); | ||
} | ||
$this->assertTrue(rmdir($bundle->basePath . DIRECTORY_SEPARATOR . 'js')); |
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 here for the rmdir method. What exactly do you want to test here? I expect some framework operations here.
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.
It's testing for directory existence.
also set it to false in AssetManager to avoid creating a lot of empty directories. fixes yiisoft#9669
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.
looked into it with little more depth.
]); | ||
$bundle->publish($am); | ||
|
||
$notNeededFilesDir = $bundle->basePath . DIRECTORY_SEPARATOR . 'css'; |
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.
looking at the code above, this reference to CSS feels a bit magical and error prone. This should be tied to $bundle->css[0]
$publishedFile = $bundle->basePath . DIRECTORY_SEPARATOR . $filename; | ||
$this->assertFileExists($publishedFile); | ||
} | ||
$this->assertTrue(is_dir($bundle->basePath . DIRECTORY_SEPARATOR . 'js')); |
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 could be tied to dirname(@bundle->js[0])
$this->assertFileExists($dstDirName . DIRECTORY_SEPARATOR . 'dir1' . DIRECTORY_SEPARATOR . 'file2.txt'); | ||
$this->assertFileExists($dstDirName . DIRECTORY_SEPARATOR . 'dir2'); | ||
$this->assertFileExists($dstDirName . DIRECTORY_SEPARATOR . 'dir3'); | ||
|
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 test should be split in more tests with clear 3 parts:
- test setup
- execution
- assertions
This will make the test a lot easier to read & to debug.
cleanup step was added in setUp() so this will still work. split test functions filehelper test
also set it to false in AssetManager to avoid creating a lot of empty
directories. fixes #9669