Change the test cases to pytest.#47
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.
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.
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 <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
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.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 27 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (3)
test/test_translation.py:72
- Corrected the typo in the test method name from 'test_missing_placehoder' to 'test_missing_placeholder'.
test/test_translation.py:19 - This class-scoped fixture is not using a class method (it uses 'self' parameter), which is incorrect for class-scoped fixtures. Class-scoped fixtures in pytest typically should not use 'self'. Instead, you should either use scope="module" with a module-level fixture function, or if you need class scope, ensure the fixture is properly configured. The current setup may lead to unexpected behavior where the fixture is not properly shared across all test methods in the class.
test/test_translation.py:44 - The translation for "foo.hi" was moved from setup_class (class-scoped) to setup_method (function-scoped). This changes the test behavior significantly. If any test modifies this translation, it could affect other tests when it was in setup_class, but now it will be reset for each test. While this improves test isolation, it's worth verifying that this behavioral change is intentional and that tests don't rely on the previous shared state behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 27 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 29 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 29 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
test/test_translation.py:70
- Fixed typo in test method name from 'test_missing_placehoder' to 'test_missing_placeholder'.
test/test_translation.py:17 - The fixture 'setup_class' uses 'self' as a parameter but class-scoped fixtures typically should not use 'self' as they are not bound to instances. This fixture should either use 'scope="class"' without 'self' parameter, or the method should be a classmethod. The correct approach would be to define it as a standalone fixture function or use '@classmethod' with '@pytest.fixture(scope="class", autouse=True)'.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return self.substitute(**kwargs) | ||
| else: | ||
| return self.safe_substitute(**kwargs) | ||
| delimiter = config.get("placeholder_delimiter") |
There was a problem hiding this comment.
The delimiter is retrieved from config on every format call (line 11). This could be problematic if the placeholder_delimiter is changed between template instantiation and format calls, potentially leading to inconsistent behavior. Consider storing the delimiter at initialization time to ensure consistent behavior.
| if isinstance(dic, dict) and len(dic) == 1 and locale in dic: | ||
| dic = dic[locale] |
There was a problem hiding this comment.
The load_translation_dic function has been refactored to avoid string concatenation by using a helper function 'join_ns'. The logic on line 90-91 attempts to unwrap a dictionary if it contains only one key matching the locale. However, this check uses 'len(dic) == 1' which will unwrap the dictionary even if it legitimately contains only one top-level key that happens to match the locale name. This could cause unexpected behavior when translation files have a single top-level key matching a locale code.
| assert translations.has("mail_number") | ||
| translated_plural = translations.get("mail_number") | ||
| assert isinstance(translated_plural, dict) | ||
| assert translated_plural["zero"] == "You do not have any mail." | ||
| assert translated_plural["one"] == "You have a new mail." | ||
| assert translated_plural["many"] == "You have %{count} new mails." |
There was a problem hiding this comment.
The test assertions on lines 188-193 are checking for 'mail_number' but should be checking for 'foo.mail_number'. The file 'foo.en.yml' is loaded with namespace 'foo' based on the filename format '{namespace}.{locale}.{format}'. These assertions will likely fail.
| delimiter = config.get("placeholder_delimiter") | ||
| pattern = re.compile( | ||
| r"{0}\{{([_a-zA-Z][_a-zA-Z0-9]*)\}}".format(re.escape(delimiter)) | ||
| ) |
There was a problem hiding this comment.
The TranslationFormatter has been refactored from using Python's string.Template class to a custom regex-based implementation. The regex pattern on line 13 compiles the pattern inside the format method on every call, which is inefficient. Consider compiling the pattern once during initialization or caching it to avoid recompilation on each format call.
|
It's great that you're trying to help but this PR is becoming too much of a mess and touches too many things.
|
2 participants
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.