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.
not sure, what do you mean? this is just to fail tests in case of permission issues.
|
|
||
| foreach ($bundle->js as $filename) { | ||
| $publishedFile = $bundle->basePath . DIRECTORY_SEPARATOR . $filename; | ||
| $this->assertTrue(unlink($publishedFile)); |
There was a problem hiding this comment.
this looks like you are testing the 'unlink' function
There was a problem hiding this comment.
It's testing for file existence.
There was a problem hiding this comment.
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.
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.
don't you want tests to be clear and simple?
There was a problem hiding this comment.
Agree with @dynasource. It's better to do it in two steps.
| $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.
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.
It's testing for directory existence.
also set it to false in AssetManager to avoid creating a lot of empty directories. fixes yiisoft#9669
| ]); | ||
| $bundle->publish($am); | ||
|
|
||
| $notNeededFilesDir = $bundle->basePath . DIRECTORY_SEPARATOR . 'css'; |
There was a problem hiding this comment.
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.
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.
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