feat: Add pan:delete command with ID-specific deletion and test case.#10
feat: Add pan:delete command with ID-specific deletion and test case.#10ruchit288 wants to merge 14 commits intopanphp:mainfrom
Conversation
There was a problem hiding this comment.
I don't think this is deleting all? The where clause in the repository says it's removing one entity or its not all together.
There was a problem hiding this comment.
You are correct. I just update the description.
| <?php | ||
|
|
||
| beforeEach(function (): void { | ||
| $this->repository = mock(AnalyticsRepository::class)->makePartial(); |
There was a problem hiding this comment.
I like the test here, the way it's broken down.
What is the performance gain in using a mock versus hitting the DB here?
I would suggest hitting the DB and confirming the row is actually deleted or test the repository method in isolation.
There was a problem hiding this comment.
@ruchit288 can you assert the database is missing the ID after deleting in these tests instead of the mock? Another way would be, leave the command class test as it is, and have a separate test for the delete of method of the repository (assert database missing the ID deleted).
You can check the merged branches, look at the feature test cases on how you can insert some data into the table. That would even prevent hard coding the id like ['id' => 1]. You can use insertGetId, which would make the tests more stable and less prone to failure.
Co-authored-by: Taghwo Millionaire <40868373+taghwo@users.noreply.github.com>
Co-authored-by: Taghwo Millionaire <40868373+taghwo@users.noreply.github.com>
There was a problem hiding this comment.
As per @nunomaduro pan:delete {id}, I think we need an argument here at the place of option.
| $id = $this->option('id'); | |
| $id = $this->argument('id'); |
Merge updates.
Removed the intermediate variable for more concise code.
There was a problem hiding this comment.
I think the client i.e the command should be the one handling this. Returning int here would be better than this string.
This PR contain:
php artisan pan:delete 1