Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve Test Coverage for github/models #328

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Improve Test Coverage for github/models #328

wants to merge 7 commits into from

Conversation

yashpandey06
Copy link
Collaborator

Screenshot 2025-01-02 022755

@yashpandey06 yashpandey06 requested a review from arkid15r as a code owner January 1, 2025 21:06
@yashpandey06 yashpandey06 marked this pull request as draft January 1, 2025 21:08
@yashpandey06 yashpandey06 marked this pull request as ready for review January 1, 2025 21:34
@arkid15r arkid15r linked an issue Jan 2, 2025 that may be closed by this pull request
Increase backend github/models coverage #282
Open
arkid15r
arkid15r requested changes Jan 2, 2025
View reviewed changes
Copy link
Collaborator

@arkid15r arkid15r left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this @yashpandey06
Please address the comments below when you get a chance:

backend/tests/github/models/issue_tests.py Outdated Show resolved Hide resolved
backend/tests/github/models/issue_tests.py Outdated Show resolved Hide resolved
backend/tests/github/models/issue_tests.py Outdated Show resolved Hide resolved
backend/tests/github/models/issue_tests.py Outdated Show resolved Hide resolved
backend/tests/github/models/issue_tests.py Outdated Show resolved Hide resolved
backend/tests/github/models/repository_tests.py Outdated Show resolved Hide resolved
backend/tests/github/models/repository_tests.py Show resolved Hide resolved
backend/tests/github/models/user_tests.py Outdated Show resolved Hide resolved
backend/tests/github/models/user_tests.py Outdated Show resolved Hide resolved
backend/tests/github/utils_tests.py Outdated Show resolved Hide resolved
@yashpandey06 yashpandey06 force-pushed the Nest-issue-282 branch 3 times, most recently from d89c98b to c413bd3 Compare January 3, 2025 08:15
@yashpandey06 yashpandey06 requested a review from arkid15r January 3, 2025 08:23
arkid15r
arkid15r requested changes Jan 3, 2025
View reviewed changes
Copy link
Collaborator

@arkid15r arkid15r left a comment

Choose a reason for hiding this comment

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

Please address all comment and leave actual marking comments as resolved to reviewers.

backend/tests/github/models/mixins/mixins_issue_tests.py Outdated Show resolved Hide resolved
backend/tests/github/models/mixins/mixins_issue_tests.py Outdated Show resolved Hide resolved
backend/tests/github/models/repository_tests.py Outdated Show resolved Hide resolved
backend/tests/github/models/user_tests.py Outdated Show resolved Hide resolved
backend/tests/github/models/user_tests.py Outdated Show resolved Hide resolved
yashpandey06
yashpandey06 commented Jan 4, 2025
View reviewed changes
backend/tests/github/models/mixins/issue_tests.py Outdated


class TestIssueIndexMixin:
CREATED_AT = "2021-09-01T00:00:00Z"
Copy link
Collaborator Author

@yashpandey06 yashpandey06 Jan 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Reason for this :
Earlier directly writing constants values into assertions were failing pre-commit checks , therefore assigned variables to them in required files .

@yashpandey06 yashpandey06 requested a review from arkid15r January 4, 2025 01:01
arkid15r
arkid15r requested changes Jan 4, 2025
View reviewed changes
backend/tests/github/models/mixins/repository_tests.py Outdated Show resolved Hide resolved
backend/tests/github/models/mixins/user_tests.py Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here -- this doesn't test actual RespsitoryIndexMixin logic.

Copy link
Collaborator Author

@yashpandey06 yashpandey06 Jan 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I used the explicit MockModel class to extend this Mixin, so we can properly test the functions and their return values.

Any references would be helpful. Also, could you please take a look at the recent commit changes?
I might be missing something, and a reference would certainly help.

@arkid15r

Copy link
Collaborator

Choose a reason for hiding this comment

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

Take a look at how you test Organization mixin -- you test actual code behavior. You don't have to mock, keep it simple and readable.

backend/tests/github/models/repository_contributor_tests.py Outdated
def test_from_github(self):
default_contribution_value = 5
repository_contributor = RepositoryContributor()
gh_label = Mock(contributions=default_contribution_value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's still named gh_label, why was this marked as resolved?

@yashpandey06 yashpandey06 requested a review from arkid15r January 4, 2025 22:25
arkid15r
arkid15r reviewed Jan 5, 2025
View reviewed changes
backend/tests/github/models/mixins/user_tests.py Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Take a look at how you test Organization mixin -- you test actual code behavior. You don't have to mock, keep it simple and readable.

backend/tests/github/models/repository_contributor_tests.py Outdated
@@ -7,26 +7,28 @@ class TestRepositoryContributor:
def test_from_github(self):
default_contribution_value = 5
repository_contributor = RepositoryContributor()
gh_label = Mock(contributions=default_contribution_value)
repository_contributor.from_github(gh_label)
gh_label_mock = Mock(contributions=default_contribution_value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You test repository contributor entity but create it from a GitHub label data?

@yashpandey06 yashpandey06 force-pushed the Nest-issue-282 branch 4 times, most recently from fc6fbee to 089525c Compare January 6, 2025 21:00
@yashpandey06 yashpandey06 force-pushed the Nest-issue-282 branch 6 times, most recently from 92f0eba to a602024 Compare January 7, 2025 10:09
arkid15r
arkid15r reviewed Jan 9, 2025
View reviewed changes
Copy link
Collaborator

@arkid15r arkid15r left a comment

Choose a reason for hiding this comment

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

@yashpandey06 please address the comments when you get a chance. This keeps us from enforcing 75% coverage threshold for the backend.

Thank you!

@arkid15r arkid15r marked this pull request as draft January 13, 2025 04:58
arkid15r
arkid15r reviewed Jan 13, 2025
View reviewed changes
Copy link
Collaborator

@arkid15r arkid15r left a comment

Choose a reason for hiding this comment

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

Let me know if you still want to complete this. I'm going to re-assign it has been open for a while with no end result.

This is a blocker to 75% coverage enforcement.
Required test coverage of 50% reached. Total coverage: 74.66%

@yashpandey06
Copy link
Collaborator Author

@arkid15r will push the latest changes today ...was busy with the other work ..Apologies for late intimation

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

@arkid15r arkid15r arkid15r requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
backend backend-tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Increase backend github/models coverage
2 participants
@yashpandey06 @arkid15r