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

create test is_filetype_supported #30

Merged
merged 4 commits into from
Nov 19, 2021

Conversation

Yoda-Canada
Copy link
Contributor

@Yoda-Canada Yoda-Canada commented Nov 19, 2021

Fix #28
test file for issue #28

@oliver-pham
Copy link
Owner

Great work 🎉 , @Yoda-Canada! Just a small issue with the name of each test. I'd rather have something as verbose as test_supported_windows_file_path than test_is_filetype_supported1.

@Yoda-Canada
Copy link
Contributor Author

Yoda-Canada commented Nov 19, 2021

I have changed all names, but there are three commits. I am confused how to merge them to one. Because I feched new commit from your origin rpo, and when I git rebase main -i, I can't find the commit which I need to merge.

@oliver-pham
Copy link
Owner

Sorry if I didn't make it clear enough, @Yoda-Canada. What I was trying to say is that you should name the test cases according to what it wants to test.
For example:

def test_supported_windows_file_path6():
    file_path = "/Users/anonymous/Documents/test"
    assert is_filetype_supported(file_path) == False, "empty extension error"

test_supported_windows_file_path6 doesn't explain what the test does. The test actually checks a Unix file path ("/Users/anonymous/Documents/test") that has no extension. Therefore, it'd better to name it test_empty_extension_unix_file_path. I think it's good practice to name each test case descriptively so that it should give developers a clue about what the test case does.

@oliver-pham
Copy link
Owner

Don't worry about rebasing for now. I can help you with that from my end.

Copy link
Owner

@oliver-pham oliver-pham left a comment

Choose a reason for hiding this comment

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

Alright! Thanks a lot for your contribution 👏

@oliver-pham oliver-pham merged commit db51ca0 into oliver-pham:main Nov 19, 2021
@Yoda-Canada
Copy link
Contributor Author

Alright! Thanks a lot for your contribution 👏

please help me to merge all commits. I always have mistakes to do it. thx.

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.

Unit test for checking supported file formats
2 participants