Added seeNumRecords to Laravel5 module #3816

Merged
merged 3 commits into from Dec 7, 2016

Conversation

Projects
None yet
2 participants
@dmoreno
Contributor

dmoreno commented Dec 5, 2016

This is equivalent to seeNumRecords in DB module

Added seeNumRecords to Laravel5 module
This is equivalent to seeNumRecords in DB module
@janhenkgerritsen

Thanks for the PR! It would be great if you could fix the small changes I indicated in this review and also add a grabNumRecords method.

src/Codeception/Module/Laravel5.php
+ */
+ protected function getQueryBuilderFromTable($table)
+ {
+ return $query = $this->app['db']->table($table);

This comment has been minimized.

@janhenkgerritsen

janhenkgerritsen Dec 5, 2016

Contributor

The $query variable is unnecessary here

@janhenkgerritsen

janhenkgerritsen Dec 5, 2016

Contributor

The $query variable is unnecessary here

src/Codeception/Module/Laravel5.php
+ if (class_exists($table)) {
+ $currentNum = $this->countModels($table, $attributes);
+ if ($currentNum != $expectedNum) {
+ $this->fail("The number of found $table ($currentNum) does not match expected number $expectedNum in with " . json_encode($attributes));

This comment has been minimized.

@janhenkgerritsen

janhenkgerritsen Dec 5, 2016

Contributor

Missing text between "in" and "with" in error message.

@janhenkgerritsen

janhenkgerritsen Dec 5, 2016

Contributor

Missing text between "in" and "with" in error message.

src/Codeception/Module/Laravel5.php
+ } else {
+ $currentNum = $this->countRecords($table, $attributes);
+ if ($currentNum != $expectedNum) {
+ $this->fail("The number of found records ($currentNum) do not match expected number $expectedNum in table $table with " . json_encode($attributes));

This comment has been minimized.

@janhenkgerritsen

janhenkgerritsen Dec 5, 2016

Contributor

Change "do not match" to "does not match" in error message.

@janhenkgerritsen

janhenkgerritsen Dec 5, 2016

Contributor

Change "do not match" to "does not match" in error message.

src/Codeception/Module/Laravel5.php
@@ -1050,6 +1075,62 @@ protected function findRecord($table, $attributes = [])
}
+ /**
+ * @param string $modelClass
+ * @param array $attributes
+ * @return array

This comment has been minimized.

@janhenkgerritsen

janhenkgerritsen Dec 5, 2016

Contributor

Return type should be integer.

@janhenkgerritsen

janhenkgerritsen Dec 5, 2016

Contributor

Return type should be integer.

src/Codeception/Module/Laravel5.php
+ /**
+ * @param string $table
+ * @param array $attributes
+ * @return array

This comment has been minimized.

@janhenkgerritsen

janhenkgerritsen Dec 5, 2016

Contributor

Return type should be integer.

@janhenkgerritsen

janhenkgerritsen Dec 5, 2016

Contributor

Return type should be integer.

@dmoreno

This comment has been minimized.

Show comment
Hide comment
@dmoreno

dmoreno Dec 7, 2016

Contributor

@janhenkgerritsen, thanks for your code review. grabNumRecords method is a good idea. I have applied all your proposed changes.

Contributor

dmoreno commented Dec 7, 2016

@janhenkgerritsen, thanks for your code review. grabNumRecords method is a good idea. I have applied all your proposed changes.

src/Codeception/Module/Laravel5.php
+ */
+ public function grabNumRecords($table, $attributes = [])
+ {
+ return (class_exists($table))? $this->countModels($table, $attributes) : $this->countRecords($table, $attributes);

This comment has been minimized.

@janhenkgerritsen

janhenkgerritsen Dec 7, 2016

Contributor

Can you remove the extra parentheses around the class_exists call?

@janhenkgerritsen

janhenkgerritsen Dec 7, 2016

Contributor

Can you remove the extra parentheses around the class_exists call?

@dmoreno

This comment has been minimized.

Show comment
Hide comment
Contributor

dmoreno commented Dec 7, 2016

@janhenkgerritsen, done!

@janhenkgerritsen

This comment has been minimized.

Show comment
Hide comment
@janhenkgerritsen

janhenkgerritsen Dec 7, 2016

Contributor

Thanks for adding the grabNumRecords method and applying the changes. I see you already pushed the change for the parentheses :)

The previous build failed, but this was not related to your changes.

Contributor

janhenkgerritsen commented Dec 7, 2016

Thanks for adding the grabNumRecords method and applying the changes. I see you already pushed the change for the parentheses :)

The previous build failed, but this was not related to your changes.

@janhenkgerritsen janhenkgerritsen merged commit 93cba3c into Codeception:2.2 Dec 7, 2016

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
semaphoreci The build passed on Semaphore.
Details
@janhenkgerritsen

This comment has been minimized.

Show comment
Hide comment
@janhenkgerritsen

janhenkgerritsen Dec 7, 2016

Contributor

I added the changes from this PR to the changelog in #3822. Thanks for your work!

Contributor

janhenkgerritsen commented Dec 7, 2016

I added the changes from this PR to the changelog in #3822. Thanks for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment