-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
chore(gae): POC - enable support for tests on Python 3 #13242
base: main
Are you sure you want to change the base?
chore(gae): POC - enable support for tests on Python 3 #13242
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @eapl-gemugami, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request is a proof of concept to enable unit tests for the appengine
directory on Python 3. The author has noticed that tests are currently being skipped in this directory on Python 3+, which could lead to problems. The changes modify conftest.py
to conditionally skip App Engine tests based on the presence of the GAE_SDK_PATH
environment variable and adds pytest
and six
to the test requirements.
Highlights
- Enable Python 3 tests: The primary goal is to enable unit tests in the
appengine
directory for Python 3, which are currently being skipped. - Conditional test skipping: The
conftest.py
file is modified to skip tests only if theGAE_SDK_PATH
environment variable is not set, instead of unconditionally skipping on Python 3. - Test requirements: The
requirements-test.txt
file is updated to includepytest
andsix
as dependencies.
Changelog
- appengine/standard/conftest.py
- Modified
pytest_ignore_collect
function to conditionally skip App Engine tests based on theGAE_SDK_PATH
environment variable. - Added a print statement to indicate when a test is skipped due to the missing
GAE_SDK_PATH
.
- Modified
- appengine/standard/hello_world/requirements-test.txt
- Added
pytest==8.3.5
andsix==1.17.0
as test dependencies.
- Added
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
In realms of code, where tests reside,
A challenge faced, with Python's stride.
App Engine sleeps, on version three,
This PR awakens, for all to see.
With SDK path, a guiding light,
Tests shall run, and code ignite.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request aims to enable unit tests for Python 3 in the appengine
directory by modifying the conftest.py
file. The changes involve skipping App Engine tests if the GAE SDK is not available and adding necessary dependencies to requirements-test.txt
. Overall, the changes seem reasonable for the stated goal.
Summary of Findings
- Clarity of Skipping Logic: The logic for skipping tests could be made more explicit and easier to understand. Consider adding a comment explaining why
six.PY3
check was removed and how the current logic achieves the same goal or a different one. - Test Environment Setup: It's important to ensure that the test environment is properly set up for Python 3. This might involve installing additional dependencies or configuring the environment in a specific way.
- Error Message Improvement: The error message for missing
GAE_SDK_PATH
could be improved to provide more guidance to the user on how to resolve the issue.
Merge Readiness
The pull request introduces changes that seem to address the issue of skipped tests on Python 3. However, it's crucial to ensure that the test environment is properly configured and that the skipping logic is clear and correct. I recommend addressing the review comments before merging. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging.
if "GAE_SDK_PATH" not in os.environ: | ||
#test_file = os.path.basename(collection_path) | ||
test_file = os.path.join(collection_path.parent.name, collection_path.name) | ||
|
||
if ".py" in test_file: | ||
print("GAE_SDK_PATH is missing! " | ||
f"Skipping App Engine test in {test_file}." | ||
) | ||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original code had a check for six.PY3
to skip tests on Python 3. This check is now removed. It would be helpful to add a comment explaining why this check is no longer needed or how the current logic handles Python 3 differently. Otherwise, it's not clear if this change is intentional or not, and what the implications are.
if "GAE_SDK_PATH" not in os.environ: | |
#test_file = os.path.basename(collection_path) | |
test_file = os.path.join(collection_path.parent.name, collection_path.name) | |
if ".py" in test_file: | |
print("GAE_SDK_PATH is missing! " | |
f"Skipping App Engine test in {test_file}." | |
) | |
return True | |
if "GAE_SDK_PATH" not in os.environ: | |
# The six.PY3 check was removed because ... | |
test_file = os.path.join(collection_path.parent.name, collection_path.name) | |
if ".py" in test_file: | |
print("GAE_SDK_PATH is missing! " | |
f"Skipping App Engine test in {test_file}." | |
) | |
return True |
print("GAE_SDK_PATH is missing! " | ||
f"Skipping App Engine test in {test_file}." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message could be improved to provide more guidance to the user. Consider suggesting how to set the GAE_SDK_PATH
environment variable or where to find the SDK.
print("GAE_SDK_PATH is missing! " | |
f"Skipping App Engine test in {test_file}." | |
) | |
print("GAE_SDK_PATH is missing! " | |
f"Skipping App Engine test in {test_file}. " | |
"Please set the GAE_SDK_PATH environment variable to the location of the App Engine SDK.") |
Description
I've been noticing that the tests are being skipped in the
appengine
directory on Python 3+I think this may lead to problems with actual code when working on the directory.
This is a Proof of concept enabling Unit Tests for Python 3, by changing the conftest.py file, to watch what Kokoro CI returns.
Checklist