Skip to content

Add python-3.7 tests#1818

Merged
freddyaboulton merged 23 commits into
mainfrom
1806-python-3.7-tests
Jul 19, 2022
Merged

Add python-3.7 tests#1818
freddyaboulton merged 23 commits into
mainfrom
1806-python-3.7-tests

Conversation

@freddyaboulton
Copy link
Copy Markdown
Collaborator

@freddyaboulton freddyaboulton commented Jul 18, 2022

Description

Some of our test dependencies do not support python 3.7 in their latest releases 😞

I saw two ways to solve this:

  1. Make all the dependencies in test/requirements.txt be valid python 3.7 and 3.9 dependencies.
  2. Create separate test files for 3.7 and 3.9 dependencies.

I opted for the first approach. I think that will be simpler to maintain going forward.

Fixes: #1806
Fixes #1807
Fixes #1814

Checklist:

  • I have performed a self-review of my own code
  • My code follows the style guidelines of this project
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@freddyaboulton freddyaboulton force-pushed the 1806-python-3.7-tests branch from fad2eb7 to 91464f1 Compare July 18, 2022 16:13
Comment thread gradio/components.py
from copy import deepcopy
from types import ModuleType
from typing import Any, Callable, Dict, List, Optional, Tuple
from typing import TYPE_CHECKING, Any, Callable, Dict, List, Optional, Tuple
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is for #1807

Comment thread test/test_documentation.py
Comment thread test/requirements.in
Comment thread test/test_utils.py
validated_data = client_response.get_validated_data()
assert client_response.is_valid() is True
assert validated_data["Host"] == "headers.jsontest.com"
@pytest.mark.asyncio
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The diff here is refactoring from deleting TestRequest and moving the tests out and using respx_mock , no other changes beyond that.

It's easier to use the respx_mock fixture when the tests are not in a unittest class.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks much more beautiful, good job!

Comment thread .circleci/config.yml
Comment thread .circleci/config.yml Outdated
orbs:
codecov: codecov/codecov@3.2.2
node: circleci/node@5.0.2
browser-tools: circleci/browser-tools@1.3.0
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is how you install the chrome driver on the new images

Docs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need chrome and chrome driver?

Comment thread gradio/components.py
Comment thread scripts/create_test_requirements.sh Outdated
else
echo "Creating requirements under test/requirements.txt using requirements.in. Please run this script from unix or wsl!"
echo "Creating requirements under test/requirements.txt using requirements.in. Please run this script from unix or wsl in a python3.7 env!"
python -c "import sys; assert sys.version_info < (3, 8) and sys.version_info >= (3, 7), 'Run from python 3.7!'" || exit 1
Copy link
Copy Markdown
Collaborator Author

@freddyaboulton freddyaboulton Jul 18, 2022

Choose a reason for hiding this comment

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

Modified this script to exit if it's not running in a python 3.7 environment so that devs do not create a requirements file that cannot be installed in python 3.7.

image

My thinking is that dependencies that work on python 3.7 also work on python 3.9. It's only as of recent that packages stopped supporting 3.7 so if anything we're just pinned to a slightly older version.

If for whatever reason this assumption does not hold in the future, we can add separate files for separate python versions but I don't think we need that now.

Copy link
Copy Markdown
Member

@abidlabs abidlabs Jul 18, 2022

Choose a reason for hiding this comment

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

EDITED version: We shouldn't restrict developers to use Python 3.7. Instead, I think it's fine to let them run scripts/create_test_requirements.sh in whichever version of Python they are using, but if they push a change to the requirements that isn't 3.7-compatible, they'll know because 3.7 CI tests will fail.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Spoke with @abidlabs about this off line and instead we'll let this script run in any python version.

If devs push deps versions that are not compatible in python 3.7 they will know because the ci will fail.

@freddyaboulton freddyaboulton marked this pull request as ready for review July 18, 2022 22:01
@freddyaboulton freddyaboulton requested review from omerXfaruq and removed request for omerXfaruq July 18, 2022 22:01
@freddyaboulton freddyaboulton requested review from abidlabs, aliabd, aliabid94, dawoodkhan82, omerXfaruq and pngwn and removed request for pngwn July 18, 2022 22:01
Comment thread test/test_documentation.py Outdated
import gradio as gr


class TestDocumentatino(unittest.TestCase):
Copy link
Copy Markdown
Member

@abidlabs abidlabs Jul 18, 2022

Choose a reason for hiding this comment

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

Noticed a typo:

Suggested change
class TestDocumentatino(unittest.TestCase):
class TestDocumentation(unittest.TestCase):

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Lol good eye!

Comment thread .circleci/config.yml
Comment thread .circleci/config.yml Outdated
@abidlabs
Copy link
Copy Markdown
Member

Thanks for addressing my suggestions @freddyaboulton! LGTM

Copy link
Copy Markdown
Contributor

@omerXfaruq omerXfaruq left a comment

Choose a reason for hiding this comment

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

LGTM! Just wondering about whether if we need chrome and chrome driver?

@omerXfaruq
Copy link
Copy Markdown
Contributor

omerXfaruq commented Jul 19, 2022

We needed chrome driver for selenium tests in the past, we probably don't need them anymore.

@freddyaboulton
Copy link
Copy Markdown
Collaborator Author

Thank you @farukozderim ! Went ahead and deleted the chrome driver install!

@freddyaboulton freddyaboulton merged commit 74d632e into main Jul 19, 2022
@freddyaboulton freddyaboulton deleted the 1806-python-3.7-tests branch July 19, 2022 14:48
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.

Use respx in unit tests Remove typing-extensions dependency Include CI tests for python 3.7

3 participants