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.

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

add unit tests for Tag class #199

Closed
wants to merge 12 commits into from
Closed

Conversation

DJStowe
Copy link

@DJStowe DJStowe commented May 21, 2024

Added unit tests just for the Tag class. Tests the following:
add_subtag
remove_subtag
remove_subtag if id is not in subtag_ids
debug_name
display_name with shorthand
display_name without shorthand
display_name with no subtag_ids

Also added tests for edge cases with display name when tags don't have names but I commented those out for now since we probably don't want to display them in that format. ex result for tag & subtag having no name " ()"

Added pytest-mock to requirements-dev.txt
I'm not sure what version to add, I tested on both v3.0.0 & v3.14.0

I have not been following the path.os changes so the header in test_tags.py might need to be changed

@CyanVoxel CyanVoxel added the Type: Tests Tests or testing related label May 21, 2024
Copy link
Collaborator

@yedpodtrzitko yedpodtrzitko 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 the PR. Ideally please add a CI job which will run the tests. Otherwise having tests no one runs is like having no tests at all.

requirements-dev.txt Outdated Show resolved Hide resolved
def test_tag3():
return Tag(id=3, name="tag3 Name", shorthand="tag3", aliases=[], subtags_ids=[], color="")

class TestTags():
Copy link
Collaborator

Choose a reason for hiding this comment

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

isnt this wrapping class superfluous?

Copy link
Author

Choose a reason for hiding this comment

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

At the moment yes, but it's needed for one of the commented out tests. I think display_name should probably be changed, but I wanted to have tests for it if we want to keep it as is. Maybe I'll comment that fixture out for now and open an issue to change display_name?

Comment on lines 159 to 160
if __name__ == "__main__":
pytest.main()
Copy link
Collaborator

@yedpodtrzitko yedpodtrzitko May 21, 2024

Choose a reason for hiding this comment

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

why is this needed?

Copy link
Author

Choose a reason for hiding this comment

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

You're right it isn't needed. I've removed it in the latest commits.

@DJStowe
Copy link
Author

DJStowe commented Jun 7, 2024

Thanks for the PR. Ideally please add a CI job which will run the tests. Otherwise having tests no one runs is like having no tests at all.

I've added runtest.yaml This is the first time I've worked with github actions but I think it's working as intended.

@yedpodtrzitko yedpodtrzitko mentioned this pull request Jun 13, 2024
@yedpodtrzitko
Copy link
Collaborator

The interface of Tag has changed significantly, and there are some tests covering most of the Tag scenarios, so I will close this, sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Tests Tests or testing related
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants