Skip to content

Add a unit test for join method coverage #630

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

joshu4lim
Copy link

[O] Wrote test for feature
[ ] Added changes to CHANGELOG.md
[ ] Bumped version number (delete if unneeded)

Changes proposed:
Hello, I'm a student and a first-time contributor.
One of the coverage missing in 'tables.py' is in the '_join' internal method, in lines 2039-2040 where it checks for an empty table. However, the 'join' method for external interface already has these checks in lines 2024-2025, preventing those lines from being covered. This makes it difficult to test '_join' by just calling 'join' as discussed in this PR.

In this unit test, I decided to modify the 'join' method to skip those checks and allow '_join' to be covered. This way, the functionality of _join can be tested without directly invoking it.

Please let me know if any changes are needed to improve this implementation.

Copy link
Member

@adnanhemani adnanhemani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really in favor of testing in such roundabout ways. If we have dead code, I would much rather remove it instead.

@joshu4lim joshu4lim requested a review from adnanhemani May 31, 2025 02:44
@joshu4lim joshu4lim closed this May 31, 2025
@joshu4lim joshu4lim reopened this May 31, 2025
@joshu4lim joshu4lim marked this pull request as draft May 31, 2025 02:52
@joshu4lim joshu4lim marked this pull request as ready for review May 31, 2025 03:04
@joshu4lim
Copy link
Author

Removed the proposed roundabout test case and removed the dead code inside the _join internal method on lines 2039-2040. Also removed lines 2041-2042 because it would also be dead code based on the same reasoning for removing L2039-2040.

@davidwagner
Copy link
Member

@adnanhemani I'll let you comment, but it looks reasonable to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants