Skip to content

Conversation

@peter279k
Copy link
Contributor

Changed log

  • add more tests
  • add the ext-curl in composer.json to check whether the curl extension is installed.

@coveralls
Copy link

Coverage Status

Coverage increased (+22.8%) to 95.441% when pulling 904f454 on peter279k:test_enhancement into d701624 on brick:master.

@coveralls
Copy link

coveralls commented Mar 4, 2018

Coverage Status

Coverage increased (+21.9%) to 94.529% when pulling 2033353 on peter279k:test_enhancement into 2837362 on brick:master.

@peter279k
Copy link
Contributor Author

peter279k commented Nov 22, 2018

Hi @BenMorel, thank you for your reply. And I appreciate your code review!.

I've almost pushed the latest commit for your recent request changes.

If having any request changes, please write down the comments to let me know!

Thanks.

$fixedArray = FixedArray::fromArray($expectedArray);
$result = $fixedArray->toArray();

$this->assertCount(4, $result);
Copy link
Member

Choose a reason for hiding this comment

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

Redundant test, covered by the next line. You can remove this line.

public function testCopyShouldThrowIOException()
{
FileSystem::write('./temp_lock_file', 'data');
FileSystem::copy('./temp_lock_file', './non_existed_dir/temp_lock_file');
Copy link
Member

Choose a reason for hiding this comment

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

Should be non_existing_dir

public function testMoveShouldThrowIOException()
{
FileSystem::write('./temp_lock_file', 'data');
FileSystem::move('./temp_lock_file', './non_existed_dir/temp_lock_file');
Copy link
Member

Choose a reason for hiding this comment

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

Should be non_existing_dir

*/
public function testDeleteShouldThrowIOException()
{
FileSystem::delete('./non_existed_dir/temp_lock_file');
Copy link
Member

Choose a reason for hiding this comment

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

Should be non_existing_dir

*/
public function testCreateDirectoryShouldThrowIOException()
{
FileSystem::createDirectory('./non_existed_dir/temp_lock_file');
Copy link
Member

Choose a reason for hiding this comment

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

Should be non_existing_dir

*/
public function testCreateLinkWithInvalidFileLink()
{
FileSystem::createLink('./invalid_link', './non_existed_dir/invalid_target');
Copy link
Member

Choose a reason for hiding this comment

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

Should be non_existing_dir

*/
public function testWriteWithInvalidFilePath()
{
FileSystem::write('./non_existed_dir/invalid_path', 'data');
Copy link
Member

Choose a reason for hiding this comment

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

Should be non_existing_dir

*/
public function testReadWithInvalidFilePath()
{
FileSystem::read('./non_existed_dir/invalid_path');
Copy link
Member

Choose a reason for hiding this comment

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

Should be non_existing_dir

@BenMorel
Copy link
Member

Filesystem tests are currently creating files/dirs in the current directory.
Could you please create a temporary directory (./tmp) in setUp(), delete it in tearDown(), and change all tests to only work with files in this directory?

At least all tests will start in a fresh environment, and the working directory will be kept clean.
Oh, and please use native PHP functions in setUp() and tearDown().

@peter279k
Copy link
Contributor Author

Hi @BenMorel, thank you for your reply.

I think the Filesystem test approach that you mention makes sense.

I will follow your approach to complete the Filesystem tests :).

@BenMorel
Copy link
Member

Sorry, I told you nonsense regarding ./tmp, I forgot that I already had setUp() and tearDown() which did exactly what I requested above, and a chdir() call which allows you to use a relative path with no prefix.

Can you please revert the previous commit, but keep the data providers?

@peter279k
Copy link
Contributor Author

@BenMorel, I've removed prefixed ./tmp path directory and keep the dataProvider :).

Please review that. Thanks.

@BenMorel
Copy link
Member

Looks good now, please just remove all ./ prefixes in front of file names, they're useless.

$this->file_put_contents('read.txt', 'read content');

$this->assertSame('read content', FileSystem::read('read'));
$this->assertSame('read content', FileSystem::read('read.txt'));
Copy link
Member

Choose a reason for hiding this comment

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

Could you please remove all the .txt you added above? The add no value to the existing tests.

@peter279k
Copy link
Contributor Author

Hi @BenMorel, thank you for your reply.

I've completely removed the .txt extension name.

@BenMorel BenMorel merged commit b8acd4b into brick:master Nov 30, 2018
@BenMorel
Copy link
Member

All good now. Thanks! 👍

@peter279k peter279k deleted the test_enhancement branch November 30, 2018 15:53
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.

3 participants