search

sublinks / sublinks-api

MIT License
68 stars 17 forks source link

#246-added unit test to search package #396

Closed Ashwinn11 closed 4 weeks ago

Ashwinn11 commented 1 month ago

fixes #246 -Added unit testing to the search package

lazyguru commented 1 month ago

@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)

Pdzly commented 1 month ago

Great job! Anything else than what @lazyguru said looks fine to me! :+1:

Ashwinn11 commented 1 month ago

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:

1) Renamed testSearchPostByUrl to testSearchPostByUrlReturnsEmptyWhenNoCrossPostsFound to clarify that it tests the case where no cross-posts are found.

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.

jgrim commented 4 weeks ago

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.

lazyguru commented 4 weeks ago

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