Add session state and dummy state handler tests #266
Conversation
joshcanhelp
force-pushed
the
add-storage-tests
branch
from
af32452
to
561eb3a
Compare
| @@ -1,44 +0,0 @@ | |||
| <?php | |||
There was a problem hiding this comment.
Renamed to SessionStateHandlerTest.php and refactored above
joshcanhelp
force-pushed
the
add-storage-tests
branch
from
561eb3a
to
45d1bd6
Compare
| */ | ||
| public function testStateIssuedCorrectly() | ||
| { | ||
| $this->assertNull($this->state->issue()); |
There was a problem hiding this comment.
how is a "null" state correct?
There was a problem hiding this comment.
This is a dummy class so null is desired in this case.
There was a problem hiding this comment.
Is there a reason to keep this dummy class in the main library's package rather than in the test package?
| public function testStateValidatesCorrectly() | ||
| { | ||
| $this->assertTrue($this->state->validate(uniqid())); | ||
| $this->assertTrue($this->state->validate(uniqid())); |
There was a problem hiding this comment.
duplicated line. If is intentional leave a comment in the code
| { | ||
| $uniqid = uniqid(); | ||
| $this->state->store($uniqid); | ||
| $this->assertEquals($uniqid, $this->store->get(SessionStateHandler::STATE_NAME)); |
There was a problem hiding this comment.
I missed this one before I guess.. why if you haven't passed a state name above, you're passing one now? Either pass always the name or never.
There was a problem hiding this comment.
I think you're misreading, these are 2 different methods. I'll name for clarity.
| */ | ||
| public function testStateIssuedCorrectly() | ||
| { | ||
| $state_issued = $this->state->issue(); |
There was a problem hiding this comment.
Issue() stores using the default name, right?
| } | ||
|
|
||
| /** | ||
| * Test that state validated properly. |
There was a problem hiding this comment.
"is" validated or "validates"
| { | ||
| $state_issued = $this->state->issue(); | ||
| $this->assertTrue($this->state->validate($state_issued)); | ||
| $this->assertNull($this->store->get(SessionStateHandler::STATE_NAME)); |
There was a problem hiding this comment.
here you test that after the validate("state"), the value gets removed. Right?
| $state_issued = $this->state->issue(); | ||
| $this->assertTrue($this->state->validate($state_issued)); | ||
| $this->assertNull($this->store->get(SessionStateHandler::STATE_NAME)); | ||
| $this->assertFalse($this->state->validate($state_issued . 'false')); |
There was a problem hiding this comment.
So if this one is checking if a different state fails the validation, this one belongs to a separate test. Since on the line above you've already proved that the stored state is missing.
tests/Store/SessionStoreTest.php
Outdated
| */ | ||
| public function testDelete() | ||
| { | ||
| self::$sessionStore->delete(self::TEST_KEY); |
There was a problem hiding this comment.
to me, this should first ensure a key is stored (store + check it exists) then delete and check it no longer exists
There was a problem hiding this comment.
$_SESSION persists though so tests above will cover that.
I did add an assertion for the get method here, though
There was a problem hiding this comment.
tests should be independent. Running whichever test alone should work. Also, if the delete() method doesn't have any logic inside it (literally an empty body) the test will still pass. You should prepare each test case / scenario before testing the actual thing.
tests/Store/SessionStoreTest.php
Outdated
| */ | ||
| public function testGet() | ||
| { | ||
| $_SESSION[self::$sessionKey] = self::TEST_VALUE; |
There was a problem hiding this comment.
to me, this should first ensure a key is not stored then add and check it now exists
joshcanhelp
force-pushed
the
add-storage-tests
branch
from
5268dac
to
1bf3395
Compare
|
@lbalmaceda - Made the delete test more isolated based on your comment. Ready for review next week! |
| @@ -9,8 +9,6 @@ branches: | |||
|
|
|||
| php: | |||
| - 5.5 | |||
| - 5.6 | |||
There was a problem hiding this comment.
@cocojoe What's the point on testing each version that comes out? Isn't php retro-compatible? IMO, just test the minimum supported version and the latest supported one. Each build runs for ~8mins. Multiply that for each version and flavor you want to test and you get how much it takes for a php PR to run CI.
| */ | ||
| public function testStateIssuedCorrectly() | ||
| { | ||
| $this->assertNull($this->state->issue()); |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
No description provided.