-
Notifications
You must be signed in to change notification settings - Fork 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
unit test added. Part2 #2
base: master
Are you sure you want to change the base?
Conversation
Run this script for testing FileLock module. Algorithm is follow: 1) script created file in current folder 2) trying to lock file 3) checking that file is locked 4) trying to read 5) checking that file is locked 6) trying to write 7) checking that file is locked 8) try to unlock file 9) checking that file is not locked 10) trying to read 11) checking that file is not locked 12) trying to write 13) checking that file is not locked echo 'done' and exit
Unittests are awesome, but could you please send them in a more "traditional" way? With PHPUnit(preferably) or with Codeception(or any other widely used for unittesting library)? |
I've added required travis and coverall integrations, so it would be easier make it properly and see results. |
sent updated tests for PHPUnit |
TravisCI shows that there are some issues. Could you please fix them? |
ok, got it |
I don't get it. Why did you rewrote files:
by same files? Why not just pull from master? It's same-same 20dd541 Did you run |
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.
Currently test doesn't cover locking functionality. Only most basic - create lock, acquire lock, release lock. It's way better then nothing, but maybe you could also add test for locking mechanics also?
tests/FileLockTest.php
Outdated
$fname = 'tests/lock-test.txt'; | ||
|
||
//create file | ||
$handle = fopen($fname, 'w') or die('ERROR[0]: Unable to open/create file "'.$fname.'"!'); |
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 why you need this part, but it should be in setUp()
, according to phpunit docs
And should not output anything for sure.
As I can see, you've added this file to repo already, so there is no need to create this file each time.
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, got it
tests/FileLockTest.php
Outdated
|
||
use PHPUnit\Framework\TestCase; | ||
|
||
$fname = 'tests/lock-test.txt'; |
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.
Should be a part of test class or variable in test method.
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.
understood
tests/FileLockTest.php
Outdated
|
||
public function testCanBeLocked() { | ||
|
||
global $fname; |
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.
You should have a VERY big and good reason to use global
keyword. And I don't see any reason to use it 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.
agreed
tests/FileLockTest.php
Outdated
$lock = new \Dorantor\FileLock($fname); | ||
|
||
//try to lock file | ||
$this->assertEquals(true, $lock->acquire(),'ERROR[1]: Result of function acquire() not correct!'. PHP_EOL); |
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.
assertEquals() is a static method, so it should be called via self
:
self::assertEquals()
Also, there is no need in message in assert - currently it doesn't add any value to test.
Also, it makes sense to replace assertEquals()
with assertTrue()
. Thus assert call will be simple and most readable.
tests/FileLockTest.php
Outdated
|
||
//try to lock file | ||
$this->assertEquals(true, $lock->acquire(),'ERROR[1]: Result of function acquire() not correct!'. PHP_EOL); | ||
/* |
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.
Why did you commit code that is commented out?
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.
Actually Git do it in automatic way after i add Travis CI to my repository. And i sow this so late after i do a lot of commits.
unit-test.php
Outdated
@@ -0,0 +1,107 @@ | |||
<?php |
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 assume this file doesn't needed anymore?
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.
yes, you are right
Hello, "Currently test doesn't cover locking functionality" - yes, i know. At the moment i have some issues with checking file LOCK in Linux system. And my tests always end with errors. So i still going to complete tests for more complex test. |
hello. i updated test file |
Run this script for testing FileLock module.
Algorithm is follow:
echo 'done' and exit