-
Notifications
You must be signed in to change notification settings - Fork 2
テストコードのリファクタリング #711
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
テストコードのリファクタリング #711
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.
Pull Request Overview
This PR refactors the test code by leveraging pytest markers to distinguish tests that access the web API from those that do not. It also updates the GitHub Actions workflow to skip webapi tests and removes redundant test files.
- Added pytest marker "@pytest.mark.access_webapi" to relevant test classes.
- Imported and used the my_backoff decorator in new test cases.
- Updated the GitHub Actions configuration to exclude webapi tests.
Reviewed Changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
tests/test_api.py | Added new test classes for my_backoff and request_logger tests |
tests/test_pydantic_models.py | Added pytest marker for webapi access tests |
tests/test_sandbox.py | Added pytest marker for webapi access tests |
.github/workflows/lint-test.yml | Modified the test command to exclude access_webapi tests |
annofabapi/api.py | Updated my_backoff decorator and related docstrings |
tests/test_api2.py | Added pytest marker for webapi access tests |
tests/test_local_resource.py | Updated environment variable removals |
tests/tests_local_api.py | Removed redundant test file |
Files not reviewed (2)
- Makefile: Language not supported
- pytest.ini: Language not supported
tests/test_api.py
Outdated
@@ -50,6 +51,63 @@ | |||
test_wrapper = WrapperForTest(api) | |||
|
|||
|
|||
class TestMyBackoff: | |||
@my_backoff | |||
def acesss_api(self, log: list[str], exception: Exception): |
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 function name 'acesss_api' appears to be misspelled. Consider renaming it to 'access_api' for clarity.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
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.
Pull Request Overview
This PR refactors the test code by switching from using file name prefixes (test_local_*) to applying the pytest marker “access_webapi” for tests that access the web API, and updates the GitHub Actions workflow to skip these tests. Key changes include:
- Adding the “access_webapi” marker to test modules and classes.
- Importing and testing the backoff decorator using the new marker.
- Modifying the lint-test workflow to set a dummy ANNOFAB_PAT environment variable and to exclude web API tests.
Reviewed Changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
tests/test_api.py | Adds multiple test classes with the “access_webapi” marker and new backoff tests. |
.github/workflows/lint-test.yml | Updates the test execution command and sets a dummy environment variable for testing. |
tests/test_pydantic_models.py | Adds the “access_webapi” marker. |
tests/test_sandbox.py | Adds the “access_webapi” marker. |
tests/test_api2.py | Adds the “access_webapi” marker. |
tests/test_resource.py | Minor adjustment to environment variable cleanup in tests. |
tests/tests_local_api.py | Removal of local API tests now superseded by the marker-based approach. |
annofabapi/api.py | Minor documentation updates in the retry/backoff methods. |
Files not reviewed (2)
- Makefile: Language not supported
- pytest.ini: Language not supported
Comments suppressed due to low confidence (2)
tests/test_api.py:367
- [nitpick] Consider renaming 'TestMy' to a more descriptive name that reflects the tested functionality, such as 'TestMyAccount' or another suitable identifier.
class TestMy:
tests/test_api.py:587
- [nitpick] Consider renaming 'Testsupplementary' to 'TestSupplementary' to improve consistency and clarity in naming conventions.
class Testsupplementary:
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.
Pull Request Overview
This PR refactors the test suite by using pytest markers to differentiate webapi-access tests from local tests, and updates the CI configuration to ignore webapi tests. Additionally, the refactoring includes minor improvements in documentation and type hints in the API module.
- Removed the tests_local_api.py file in favor of marker-based test segregation.
- Added pytest.mark.access_webapi markers to appropriate test modules.
- Updated GitHub Actions workflow to set a dummy ANNOFAB_PAT and to skip webapi tests.
- Revised docstrings and type annotations in annofabapi/api.py for clarity.
Reviewed Changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
tests/tests_local_api.py | Removed local API test file in favor of marker-based test segregation. |
tests/test_sandbox.py | Added a marker and comment to denote that this module accesses webapi. |
tests/test_resource.py | Added cleanup for the ANNOFAB_PAT environment variable before testing. |
tests/test_pydantic_models.py | Added a marker to denote that this module accesses webapi. |
tests/test_api2.py | Added a marker and comment to denote webapi tests. |
tests/test_api.py | Added multiple markers for webapi tests and introduced new tests for my_backoff. |
annofabapi/api.py | Improved docstring details and type hints for the backoff functionality. |
.github/workflows/lint-test.yml | Updated the test command to ignore webapi tests and ensured test env setup. |
Files not reviewed (2)
- Makefile: Language not supported
- pytest.ini: Language not supported
Comments suppressed due to low confidence (1)
annofabapi/api.py:220
- [nitpick] The function name 'fatal_code' can be ambiguous; consider renaming it to 'should_give_up' or a similar name to more clearly convey its purpose.
def fatal_code(e: Exception) -> bool:
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.
Pull Request Overview
This PR refactors test code by replacing filename prefixes with pytest markers so that tests which access the web API can be clearly distinguished and excluded from routine runs (GitHub Actions now ignores tests marked with "access_webapi"). Key changes include:
- Removing the tests/tests_local_api.py file that contained local-only tests.
- Adding the pytest marker "access_webapi" to test modules that interact with the web API.
- Updating the GitHub Actions workflow to exclude web API tests and setting a dummy ANNOFAB_PAT env variable.
Reviewed Changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
tests/tests_local_api.py | Removed local tests that are now covered by markers. |
tests/test_sandbox.py, tests/test_pydantic_models.py, tests/test_api2.py, tests/test_api.py | Added pytest marker "access_webapi" to indicate webapi access tests. |
tests/test_resource.py | Removed ANNOFAB_PAT from environment prior to testing credentials. |
annofabapi/api.py | Updated the docstring and refactored the inner function name used in the my_backoff decorator. |
.github/workflows/lint-test.yml | Configured workflow to skip webapi tests and provide a dummy ANNOFAB_PAT variable. |
Files not reviewed (2)
- Makefile: Language not supported
- pytest.ini: Language not supported
Comments suppressed due to low confidence (1)
annofabapi/api.py:201
- Consider capitalizing 'true' to 'True' in the docstring to align with Python's boolean literal conventions.
Returns:
trueならばリトライする
webapiにアクセスしないテストは、
test_local_*
というプレフィックスをファイル名に付けていました。1個のテストファイルにwebapiにアクセスするテストメソッドとそうでないテストメソッドがあるので、pytestのmarker機能を使って、webapiのテストかどうかを表すようにしました。
そして、GitHub Actionsではwebapiのテストを無視するようにします。