Add tests, docblocks for Logs endpoints #270
Conversation
joshcanhelp
force-pushed
the
add-tests-docs-pagination-logs
branch
from
e250de5 to
83079ef
Compare
| */ | ||
| public function search($params = []) | ||
| public function search(array $params = []) |
There was a problem hiding this comment.
what's the array word doing here?
There was a problem hiding this comment.
Type hinting, will throw an error if an array is not passed in.
tests/API/Management/LogsTest.php
Outdated
| */ | ||
| public function testGetOne() | ||
| { | ||
| $one_log = self::$api->get(self::$log_id); |
There was a problem hiding this comment.
this value is set on the testLogSearch test. So don't depend on that. The order of execution of this tests shouldn't be a premise. e.g. Running this test alone would fail. If you want to achieve something like that, merge both calls in a single test.
tests/API/Management/LogsTest.php
Outdated
| $search_results = self::$api->search(); | ||
| $this->assertNotEmpty($search_results); | ||
|
|
||
| // Get the 10th log entry to test pagination in self::testLogSearchPagination(). |
There was a problem hiding this comment.
I don't see this being used in the test testLogSearchPagination as this comment mentions. Anyway, it shouldn't depend on it. Please write tests that are independent from each other.
tests/API/Management/LogsTest.php
Outdated
| ]); | ||
| $this->assertNotEmpty($search_results); | ||
| $this->assertNotEmpty($search_results[0]['date']); | ||
| $this->assertNotEmpty($search_results[0]['log_id']); |
There was a problem hiding this comment.
Are you missing _id on purpose?
There was a problem hiding this comment.
Yes but not a good reason ... I'll add that :)
| * | ||
| * @return void | ||
| */ | ||
| public function testLogSearchPagination() |
There was a problem hiding this comment.
I don't understand how this one is "testing pagination". It only does a request and asserts that the response size is 10 items (per_page). Is not like other tests you've made before where you first ask for the first page of 2 items, and then ask for the second page of 1 items and compare that call 1[1].id==call 2[0].id
joshcanhelp
force-pushed
the
add-tests-docs-pagination-logs
branch
from
0197314 to
25f6517
Compare
tests/API/Management/LogsTest.php
Outdated
| 'fields' => '_id,log_id', | ||
| 'include_fields' => true, | ||
|
|
||
| // First page of 10 results. |
There was a problem hiding this comment.
comment should be 2 results
tests/API/Management/LogsTest.php
Outdated
| $this->assertCount($expected_count, $search_results_1); | ||
|
|
||
| // Now get one page of a single result and make sure it matches the second result above. | ||
| $expected_count = $expected_count / 2; |
There was a problem hiding this comment.
this math seems a little bit odd since changing the $expected_count value to something else will definitely mess up the whole test. I think it's clearer to just use "1" here and in the lines below. More straightforward
joshcanhelp
force-pushed
the
add-tests-docs-pagination-logs
branch
from
25f6517 to
a472085
Compare
… skip verify email sent
|
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. |
fieldsand pagination