-
Notifications
You must be signed in to change notification settings - Fork 47
Change the test cases to pytest. #47
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
base: master
Are you sure you want to change the base?
Conversation
…structure - Deleted old test files: `__init__.py`, `loader_tests.py`, `translation_tests.py`, and `run_tests.py`. - Removed associated resource files for settings and translations. - Created new test files in the `test` directory to maintain the same test coverage. - Ensured all necessary resources are replicated in the new structure for continued functionality.
|
@danhper Both the check and build workflows are fine; the only issue is the inability to upload the coverage report due to missing secrets. I think we can proceed to release the next version. |
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 pull request migrates the test suite from unittest to pytest, expands Python version support to 3.8-3.13, and adds GitHub workflow automation for issue management and auto-assignment.
Changes:
- Converts test files from unittest to pytest style (assertions, fixtures, and test decorators)
- Updates CI configurations (.travis.yml, .github/workflows/ci.yml) to use pytest as the test runner
- Adds GitHub Actions workflows for auto-assigning issues/PRs and tracking top issues
- Removes legacy test infrastructure (run_tests.py, i18n.tests package)
Reviewed changes
Copilot reviewed 9 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_translation.py | Converted from unittest.TestCase to pytest style with fixtures and assert statements |
| test/test_loader.py | Partially migrated to pytest with mixed unittest/pytest decorators |
| test/resources/translations/*.json | Added new translation test resource files for test coverage |
| setup.py | Removed test_suite configuration and references to old test structure |
| pyproject.toml | Added pytest dependency and updated package discovery |
| .travis.yml | Updated Python versions and test commands to use pytest |
| .github/workflows/ci.yml | Modified to run pytest instead of unittest |
| .github/workflows/auto-assign.yml | New workflow for auto-assigning issues and PRs |
| .github/workflows/issues-top.yml | New workflow for tracking and labeling top issues |
| i18n/tests/run_tests.py | Removed legacy test runner script |
Comments suppressed due to low confidence (4)
test/test_translation.py:55
- Redundant import statement. pytest is already imported at the module level (line 7), so this import inside the test method is unnecessary and should be removed.
test/test_loader.py:8 - Incomplete migration from unittest to pytest. While pytest is imported and many assertions have been converted, unittest is still being imported and several test methods still use unittest.skipUnless decorators (lines 65, 74, 91, 120, 151, 161, 174, 181, 189, 197, 214, 231, 246) instead of pytest.mark.skipif. This mixing of frameworks should be avoided - all conditional skips should use the pytest decorator for consistency.
test/test_translation.py:19 - Incorrect pytest fixture signature. Class-scoped fixtures in pytest should have
selfas the first parameter when used as instance methods, notcls. The correct signature should bedef setup_class(self):or use a module-level fixture with@pytest.fixture(scope="class"). As written, this will cause issues when pytest tries to inject the fixture.
test/test_translation.py:19 - Normal methods should have 'self', rather than 'cls', as their first parameter.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Copilot reviewed 10 out of 24 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (6)
test/test_translation.py:45
- Duplicate translation addition. The translation "foo.hi" is added in both the class-scoped fixture (line 23) and the method-scoped fixture (line 45). This duplication is unnecessary and could lead to confusion about which value is actually being used in tests. Consider removing it from one of the fixtures to avoid redundancy.
test/test_loader.py:8 - The unittest import is no longer needed since the file has been migrated to pytest. While unittest.skipUnless decorators remain in use (which is the subject of a separate comment about inconsistent migration), the unittest module import can be removed once those decorators are fully migrated to pytest.mark.skipif.
test/test_translation.py:55 - Redundant import statement. The module 'pytest' is already imported at the top of the file (line 7), so this local import is unnecessary and should be removed.
test/test_loader.py:259 - Inconsistent use of unittest and pytest skip decorators. While some tests have been converted to use pytest.mark.skipif (lines 49, 56), many other tests in this file still use the unittest.skipUnless decorator (lines 65, 74, 91, 120, 151, 161, 174, 181, 189, 197, 214, 231, 246). For consistency with the pytest migration, all skip decorators should be converted to pytest.mark.skipif format.
test/test_translation.py:19 - Inconsistent parameter naming in fixture. The method-scoped fixture on line 40 uses 'self' while this class-scoped fixture uses 'cls'. For consistency and clarity, both fixtures should use 'self' as the first parameter, following the standard pytest convention for fixtures in test classes.
test/test_translation.py:19 - Normal methods should have 'self', rather than 'cls', as their first parameter.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
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
Copilot reviewed 13 out of 27 changed files in this pull request and generated 14 comments.
Comments suppressed due to low confidence (1)
test/test_translation.py:72
- The test method name contains a typo: "test_missing_placehoder" should be "test_missing_placeholder" (missing the letter "l"). While this is an existing typo from the unittest version that was preserved during the migration, it should be corrected for consistency with the other test names and proper spelling.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| @pytest.mark.skipif(not json_available, reason="json library not available") | ||
| def test_search_translation_without_ns_nested_dict__two_levels_neting__default_locale(): |
Copilot
AI
Jan 19, 2026
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 test method name contains a typo: "neting" should be "nesting". This typo appears to have been preserved from the original unittest version, but should be corrected to "test_search_translation_without_ns_nested_dict__two_levels_nesting__default_locale" for proper spelling and clarity.
| assert translations.has("mail_number") | ||
| translated_plural = translations.get("mail_number") |
Copilot
AI
Jan 19, 2026
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 test assertion is checking for "mail_number" but based on the old test file, it should be checking for "foo.mail_number" instead. The old unittest version used self.assertTrue(translations.has("foo.mail_number")) and translations.get("foo.mail_number"). This change appears to have dropped the "foo." namespace prefix, which may cause the test to fail or test the wrong behavior.
| assert translations.has("mail_number") | |
| translated_plural = translations.get("mail_number") | |
| assert translations.has("foo.mail_number") | |
| translated_plural = translations.get("foo.mail_number") |
| "foo.en.yml", os.path.join(RESOURCE_FOLDER, "translations") | ||
| ) | ||
|
|
||
| assert translations.has("normal_key") |
Copilot
AI
Jan 19, 2026
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 test assertion is checking for "normal_key" but based on the old test file, it should be checking for "foo.normal_key" instead. The old unittest version used self.assertTrue(translations.has("foo.normal_key")). This change appears to have dropped the "foo." namespace prefix, which may cause the test to fail or test the wrong behavior.
| ) | ||
|
|
||
| assert translations.has("normal_key") | ||
| assert translations.has("parent.nested_key") |
Copilot
AI
Jan 19, 2026
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 test assertion is checking for "parent.nested_key" but based on the old test file, it should be checking for "foo.parent.nested_key" instead. The old unittest version used self.assertTrue(translations.has("foo.parent.nested_key")). This change appears to have dropped the "foo." namespace prefix, which may cause the test to fail or test the wrong behavior.
| assert translations.has("parent.nested_key") | |
| assert translations.has("foo.parent.nested_key") |
| config.set("file_format", "json") | ||
|
|
||
| resource_loader.search_translation("bar.baz.qux") | ||
| assert translations.has("foo") |
Copilot
AI
Jan 19, 2026
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 test assertion is checking for "foo" but based on the old test file, it should be checking for "bar.baz.qux" instead. The old unittest version used self.assertTrue(translations.has("bar.baz.qux")). This incorrect assertion will not properly test the expected behavior of the search_translation function.
| assert translations.has("foo") | |
| assert translations.has("bar.baz.qux") |
| fd = open("{}/{}".format(tmp_dir_name, memoization_file_name), "w") | ||
| fd.write("en:\n key: value") | ||
| fd.close() |
Copilot
AI
Jan 19, 2026
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.
File may not be closed if this operation raises an exception.
| fd = open("{}/{}".format(tmp_dir_name, memoization_file_name), "w") | |
| fd.write("en:\n key: value") | |
| fd.close() | |
| with open("{}/{}".format(tmp_dir_name, memoization_file_name), "w") as fd: | |
| fd.write("en:\n key: value") |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
This pull request updates the project's CI and testing infrastructure to use
pytestinstead ofunittest, expands Python version support, and improves GitHub workflow automation for issue and PR management. These changes modernize the testing approach, streamline CI, and introduce automated assignment and issue tracking features.Testing and CI modernization:
unittesttopytestin both CI workflows (.github/workflows/ci.yml,.travis.yml) and removes the legacyrun_tests.pyscript. Also, updates dependencies to includepytestinpyproject.toml. [1] [2] [3] [4]pyproject.tomlandsetup.pyto reflect changes in test dependencies and package discovery, ensuring compatibility with the new test runner and package structure. [1] [2]Python version support:
.travis.ymlto include 3.9 through 3.13, dropping support for older versions.GitHub workflow automation:
.github/workflows/auto-assign.yml) to automatically assign new issues and pull requests to a specific user..github/workflows/issues-top.yml) to check for open issues and display/label top issues, bugs, features, and pull requests using a dashboard.