#246-added unit test to search package #396
Conversation
| int pageSize = 10; | ||
| Sort sort = Sort.by("id"); | ||
|
|
||
| Page<Person> expectedPage = new PageImpl<>(Collections.emptyList(), PageRequest.of(page, pageSize, sort), 0); |
There was a problem hiding this comment.
Either this test should return results or the test should be renamed to indicate it doesn't return results when no match is found (a valid test case) and then a test should be added that handles results being found. As it is now, this test is a little misleading
|
@Ashwinn11 thank you for contributing. I added one comment to the review but I would also like to call out that (IMO) this should not close #246 when merged (if merged as-is) since it only adds coverage for 2 methods and there are 5 methods in the class. Yes, it technically meets the requirement of "Add unit tests to Search package" but I believe the intent of the issue was to try to cover as much of the class as possible with at least "happy path" tests (I don't expect 100% coverage) |
|
Great job! Anything else than what @lazyguru said looks fine to me! 👍 |
|
Hi @lazyguru ,Thank you for your feedback and suggestions . I have made the following updates to the test class based on your comments: Renamed the Test Method:
2)Added a New Test for When Results Are Found: Added a new test method testSearchPostByUrlWithCrossPostsReturnsResults to handle the case when cross-posts are found and results are returned. |
lazyguru
left a comment
lazyguru
left a comment
There was a problem hiding this comment.
This is a good start that we can improve on in future PRs. Thank you for this as we had no coverage at all before.
If we need more work a new ticket needs created to finish the work. I don't want it to get lost. |
Yup, I got distracted after merging and making that comment so never created the follow-up ticket. Will do that now |
4 participants
fixes #246
-Added unit testing to the search package