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

Refactoring to replace unittests with pytests #2610

Merged
merged 16 commits into from
Nov 8, 2022
Merged

Refactoring to replace unittests with pytests #2610

merged 16 commits into from
Nov 8, 2022

Conversation

abidlabs
Copy link
Member

@abidlabs abidlabs commented Nov 6, 2022

I started going through our tests and converting them from unittest to pytest, when I noticed something wrong -- some of our tests were passing even though they should not have been. It seems that pytest was letting async tests automatically pass if they were written as part of a unittest class.

For example, if you change the TestBlocksMethods::test_dict_inputs_in_config test in test_blocks.py to anything else, you'll find that it still passes:

    async def test_dict_inputs_in_config(self):
        1/0

This is not the case if we change the tests to be pure pytests. So I went through and converted all of our tests to pytest. I noticed some other tests that should not have been passing, either due to the tests being incorrect or there being bugs in our codebase. Nothing too major, but here are the changes I made:

  • Many tests in test_components.py passed in a list to Interface.__call__ instead of the input parameters as separate arguments. This is not correct and the tests have been fixed.
  • In test_components.py, several tests used .serialize() in a way that is not supported or used in our library anymore. They've been rewritten or removed.
  • Many of the components were not raising a ValueError() when passed an invalid argument for type. They now correctly do this.

All of the above issues are now fixed in the library, and all of our tests have been converted to pytest format.

Closes: #1784

@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2022

All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-2610-all-demos

@freddyaboulton
Copy link
Collaborator

@abidlabs I noticed on Friday that pytest emits some warnings along the lines of “coroutine test_….. was never awaited”. Which means those tests are never really checked 😬

Confusingly, these tests have “@pytest.mark.asyncio”.

I wonder if this PR will get rid of those will get rid of those warnings.

@abidlabs
Copy link
Member Author

abidlabs commented Nov 6, 2022

Exactly @freddyaboulton it seems that some async tests are not running at all! l elaborate in a follow up message

@abidlabs abidlabs changed the title Just some refactoring to replace unittests with pytests Refactoring to replace unittests with pytests Nov 6, 2022
@abidlabs abidlabs marked this pull request as ready for review November 6, 2022 19:54
@abidlabs
Copy link
Member Author

abidlabs commented Nov 6, 2022

I've updated the original message with more details, and this is now ready for review

Copy link
Collaborator

@freddyaboulton freddyaboulton 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 fix @abidlabs ! I think the fact some components now raise ValueError about invalid type parameter values is worthy of a mention in the changelog though.

@@ -1,14 +1,20 @@
"""
Tests for all of the componets defined on components.lpy. They are divided into two types of tests:
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit typo: components.lpy

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah thanks

@abidlabs
Copy link
Member Author

abidlabs commented Nov 8, 2022

That's fair, thanks @freddyaboulton for the review! Will add a changelog and merge in to avoid conflicts

@abidlabs abidlabs merged commit 010827e into main Nov 8, 2022
@abidlabs abidlabs deleted the pytests branch November 8, 2022 00:37
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.

Refactor tests to use pytest fixtures and plain asserts
2 participants