uv #196
uv #196
Conversation
WalkthroughThis pull request updates multiple configuration and workflow files. The CI workflows now install dependencies using Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
utilities/axe_helper.py (1)
14-14: Type annotation updated to use built-indicttypeThe change from
Dict[str, int]todict[str, int]aligns with modern Python type hinting practices (Python 3.9+) where built-in collection types can be used directly.However, there's a missing explicit handling of the
Nonedefault value in the type annotation:- self, page: Page, maximum_allowed_violations_by_impact: dict[str, int] = None + self, page: Page, maximum_allowed_violations_by_impact: dict[str, int] | None = None🧰 Tools
🪛 Ruff (0.8.2)
14-14: PEP 484 prohibits implicit
OptionalConvert to
T | None(RUF013)
tests/conftest.py (2)
52-52: Updated type annotation to use built-indicttypeThe change from
Dicttodictaligns with modern Python type hinting practices (Python 3.9+).Consider adding a return type annotation as suggested by static analysis:
-def browser_context_args( - browser_context_args: dict, base_url: str, request: SubRequest -): +def browser_context_args( + browser_context_args: dict, base_url: str, request: SubRequest +) -> dict:
91-91: Updated type annotation to use built-indicttypeThe change from
Dicttodictaligns with modern Python type hinting practices (Python 3.9+).Consider adding a return type annotation as suggested by static analysis:
-def browser_type_launch_args(browser_type_launch_args: dict, playwright: Playwright): +def browser_type_launch_args(browser_type_launch_args: dict, playwright: Playwright) -> dict:🧰 Tools
🪛 Ruff (0.8.2)
91-91: Missing return type annotation for public function
browser_type_launch_args(ANN201)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.github/workflows/devRun.yml(1 hunks).github/workflows/nightly.yml(1 hunks)README.md(2 hunks)pyproject.toml(2 hunks)tests/accesability_test.py(0 hunks)tests/checkout_test.py(0 hunks)tests/conftest.py(7 hunks)tests/inventory_test.py(0 hunks)utilities/axe_helper.py(1 hunks)
💤 Files with no reviewable changes (3)
- tests/inventory_test.py
- tests/checkout_test.py
- tests/accesability_test.py
🧰 Additional context used
🧬 Code Definitions (1)
tests/conftest.py (1)
utilities/axe_helper.py (1)
AxeHelper(9-52)
🪛 Ruff (0.8.2)
utilities/axe_helper.py
14-14: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
tests/conftest.py
50-50: scope='function' is implied in @pytest.fixture()
Remove implied scope argument
(PT003)
51-51: Missing return type annotation for public function browser_context_args
(ANN201)
91-91: Missing return type annotation for public function browser_type_launch_args
(ANN201)
🪛 markdownlint-cli2 (0.17.2)
README.md
56-56: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🔇 Additional comments (13)
tests/conftest.py (1)
47-47:✅ Verification successful
Changed fixture behavior from
yieldtoreturnThe fixture implementation has been changed from using
yieldtoreturn. This is a functional change as yield-based fixtures can have teardown code after the yield statement, while return-based fixtures cannot. Ensure this change was intentional and doesn't impact any teardown logic that might have been previously required.
🏁 Script executed:
#!/bin/bash # Check if there were any teardown operations after the yield that might have been lost git show HEAD~1:tests/conftest.py | grep -A 10 "def axe_playwright" | grep -A 10 "yield"Length of output: 88
Confirmed Fixture Change is Safe – No Teardown Logic Lost
The verification indicates that the previous implementation in
tests/conftest.pydid not contain ayield-based teardown section. Since no teardown code was present in HEAD~1 alongside the fixture definition, the intentional change from usingyieldto a directreturn(i.e.,return AxeHelper(Axe())) is safe and does not impact any teardown behavior..github/workflows/nightly.yml (1)
50-50:❓ Verification inconclusive
Updated dependency installation command
The command has been changed from
uv pip sync uv.locktouv sync --all-extras --dev. This changes the dependency installation approach to include all extras and development dependencies, which might install additional packages compared to the previous approach that used a lock file.
🏁 Script executed:
#!/bin/bash # Check if this is a standard uv command and what it does uv --help | grep -A 5 "sync" || echo "Command not found in help" # Check if pyproject.toml defines extras that would be included grep -A 10 "\[project.optional-dependencies\]" pyproject.toml || echo "No optional dependencies section found" # Check for the presence of uv.lock file that was previously used ls -la uv.lock 2>/dev/null || echo "uv.lock file not found"Length of output: 408
Action Required: Verify 'uv' Command Availability and Optional Dependencies
The dependency installation command in the nightly workflow has been updated to:
uv sync --all-extras --devinstead of the previous:
uv pip sync uv.lockThis change is intended to install all extras and development dependencies. However, based on our verification:
uvCommand Presence: The output ofuv --helpreturned "Command not found in help", which suggests that theuvcommand may not be installed or not available in the current PATH. Please ensure that this command is correctly setup in your environment.Optional Dependencies: The search in
pyproject.tomldid not reveal any[project.optional-dependencies]section. This means that the--all-extrasflag currently has no effect; if extras are intended to be installed, the configuration may need to be updated accordingly.Legacy Lock File: The
uv.lockfile is still present, even though it’s no longer being used by the new command. Consider removing it if it’s no longer required to prevent any potential confusion.Please review these points to confirm that the revised approach aligns with the intended dependency management strategy.
.github/workflows/devRun.yml (1)
24-24: Updated dependency installation commandThe command has been changed from
uv pip sync uv.locktouv sync --all-extras --dev. This change is consistent with the update in the nightly workflow file, now including all extras and development dependencies rather than using a lock file.According to the PR summary, the automatic formatting steps using Black and isort have also been removed from this workflow, which aligns with this dependency management approach change.
README.md (3)
7-8: Badge Update: New UV and Ruff badges added.
The new badge URLs for uv and Ruff align with the project's updated dependency management and quality tools. Please verify that these endpoints remain reliable and display the correct information.
53-53: Windows Command Update: Sync command revised.
The installation instruction now usesuv sync --all-extras --devto ensure that all extras and development dependencies are installed on Windows. Confirm that this command works as intended in your Windows environment.
60-61: MacOS Command Update: Activation and sync commands updated.
Switching tosource .venv/bin/activatefor virtual environment activation and usinguv sync --all-extras --devfor dependency installation streamlines the MacOS setup process. Please test these changes on MacOS to ensure they provide a smooth setup experience.pyproject.toml (7)
1-3: Build-System Update: Simplified requirements configuration.
The[build-system]section now specifiesrequires = ["hatchling"], which simplifies the build configuration. Ensure that this meets all build requirements for the project.
6-9: Dependency Groups Update: Development dependencies revised.
Thedevdependency group now includes"ruff==0.11.4"and"pre-commit==4.2.0", reflecting the removal ofblackandisortin favor of Ruff. This change supports a modern code quality toolchain—please verify that all pre-commit configurations align with these updates.
11-34: Project Metadata Reorganization: Updated project details.
The[project]section has been reorganized with updated dependency versions and metadata. This reordering improves readability and maintenance. Double-check that the new versions (e.g.,playwright==1.51.0,pytest==8.3.5) are intentional and compatible with the project.
41-44: isort Configuration: Configuration relocated without changes.
The[tool.isort]section has been moved but its configuration remains unchanged. Confirm that using the"black"profile for isort continues to meet the project's import formatting requirements.
45-70: Pytest.ini Options Block: No significant changes noted.
The[tool.pytest.ini_options]section is well-organized, and while its arrangement has been updated, no functional changes are apparent. Please verify that all test options (such as allure settings and log configurations) continue to operate as expected.
71-81: Ruff Configuration: New linting tool configuration added.
The addition of the[tool.ruff]section sets up Ruff with custom exclusions, ignored codes, a defined line length, and a target version. This is a robust configuration for ensuring quality in your codebase. Ensure that these settings reflect the desired coding guidelines.
82-84: Ruff Format Options: Formatting settings defined.
The[tool.ruff.format]section configures options likedocstring-code-formatandquote-style. These settings should help maintain consistent code formatting across the project.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/conftest.py (4)
51-51: Type annotation updated from Dict to dictThe type annotation has been updated from
Dicttodict, which is the preferred way in modern Python (3.9+).While this change is good, the static analysis tool also suggests adding a return type annotation to this function.
-def browser_context_args(browser_context_args: dict, base_url: str, request: SubRequest): +def browser_context_args(browser_context_args: dict, base_url: str, request: SubRequest) -> dict:🧰 Tools
🪛 Ruff (0.8.2)
51-51: Missing return type annotation for public function
browser_context_args(ANN201)
89-89: Type annotation updated from Dict to dictThe type annotation has been updated from
Dicttodict, which is the preferred way in modern Python (3.9+).While this change is good, the static analysis tool also suggests adding a return type annotation to this function.
-def browser_type_launch_args(browser_type_launch_args: dict, playwright: Playwright): +def browser_type_launch_args(browser_type_launch_args: dict, playwright: Playwright) -> dict:🧰 Tools
🪛 Ruff (0.8.2)
89-89: Missing return type annotation for public function
browser_type_launch_args(ANN201)
50-50: Remove implied scope parameterThe static analysis tool suggests removing the implied
scope='function'argument as it's the default value for pytest fixtures.-@pytest.fixture(scope="function") +@pytest.fixture()🧰 Tools
🪛 Ruff (0.8.2)
50-50:
scope='function'is implied in@pytest.fixture()Remove implied
scopeargument(PT003)
66-66: Redundant return section in docstringThe docstring contains two identical "Returns:" sections, which is redundant. Consider removing one of them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.pre-commit-config.yaml(2 hunks)pages/login_page.py(1 hunks)tests/checkout_test.py(1 hunks)tests/conftest.py(7 hunks)tests/inventory_test.py(1 hunks)tests/login_test.py(1 hunks)utilities/axe_helper.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/inventory_test.py
- tests/checkout_test.py
🧰 Additional context used
🧬 Code Definitions (2)
pages/login_page.py (1)
enums/User.py (1)
User(4-8)
tests/conftest.py (1)
utilities/axe_helper.py (1)
AxeHelper(9-49)
🪛 Ruff (0.8.2)
pages/login_page.py
21-21: Missing return type annotation for public function login
Add return type annotation: None
(ANN201)
21-21: Missing docstring in public method
(D102)
tests/conftest.py
50-50: scope='function' is implied in @pytest.fixture()
Remove implied scope argument
(PT003)
51-51: Missing return type annotation for public function browser_context_args
(ANN201)
89-89: Missing return type annotation for public function browser_type_launch_args
(ANN201)
tests/login_test.py
37-37: Missing return type annotation for public function test_login_error
Add return type annotation: None
(ANN201)
37-37: Missing docstring in public method
(D102)
37-37: Unused method argument: page
(ARG002)
utilities/axe_helper.py
14-14: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🪛 GitHub Actions: Pre merge test
pages/login_page.py
[error] 21-21: TypeError: unsupported operand type(s) for |: 'module' and 'type'
tests/login_test.py
[error] 6-6: Error collecting tests: TypeError: unsupported operand type(s) for |: 'module' and 'type'
🔇 Additional comments (7)
utilities/axe_helper.py (1)
33-35: Simplified Counter instantiationThe Counter initialization has been simplified by removing unnecessary line breaks, making the code more concise while maintaining the same functionality.
.pre-commit-config.yaml (3)
38-45: Great migration to Ruff for linting and formattingReplacing multiple tools (black, autoflake, isort, pyupgrade) with Ruff is a good modernization step. Ruff combines the functionality of multiple linters and formatters into a single, faster tool.
The configuration with both linting and formatting hooks is properly set up, and the
continue_on_error: trueflag ensures a smoother developer experience.
56-56: Improved formatting for additional_dependenciesThe formatting adjustment for consistency is appropriate.
62-62: Improved formatting for pretty-format-toml argsThe formatting adjustment for consistency is appropriate.
tests/login_test.py (1)
37-37: Method signature formatting is fine, but pipeline is failingThe formatting change to a single-line method signature is acceptable. However, the pipeline is failing with a type error related to the usage of the pipe (|) operator in imported files.
The error from the pipeline:
TypeError: unsupported operand type(s) for |: 'module' and 'type'on line 6 suggests issues with the LoginPage import, which likely stems from the type annotation changes in login_page.py.The static analysis also indicates a few improvements:
- The
pageparameter appears to be unused in the method body- Missing return type annotation (should be
-> None)- Missing docstring for the method
Consider addressing these issues:
@allure.title("Login with invalid credentials test") - def test_login_error(self, page: Page, username: str, password: str, expected_error: str): + def test_login_error(self, page: Page, username: str, password: str, expected_error: str) -> None: + """Test that appropriate error messages are displayed when login fails due to invalid credentials.""" self.login_page.login(username, password) expect(self.login_page.error_message).to_have_text(expected_error)If the
pageparameter is truly unused, it could be removed or prefixed with an underscore to indicate it's not being used.🧰 Tools
🪛 Ruff (0.8.2)
37-37: Missing return type annotation for public function
test_login_errorAdd return type annotation:
None(ANN201)
37-37: Missing docstring in public method
(D102)
37-37: Unused method argument:
page(ARG002)
tests/conftest.py (2)
47-47: Function behavior change from generator to standard returnThe fixture has been changed from using
yieldtoreturn. This changes the behavior of the fixture from a generator that potentially allows teardown operations to a standard function return. Since there's no teardown code after the originalyield, this change doesn't affect functionality but makes the code clearer.
45-45: Good addition of return type annotationAdding the return type annotation
-> AxeHelperimproves code readability and type safety.
| def check_accessibility( | ||
| self, page: Page, maximum_allowed_violations_by_impact: Dict[str, int] = None | ||
| self, page: Page, maximum_allowed_violations_by_impact: dict[str, int] = None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improved type hinting with modern syntax
The change from Dict[str, int] to dict[str, int] is appropriate and follows modern Python typing practices (Python 3.9+) where built-in container types can be used directly as type annotations.
However, there's a static analysis issue to fix:
- def check_accessibility(
- self, page: Page, maximum_allowed_violations_by_impact: dict[str, int] = None
+ def check_accessibility(
+ self, page: Page, maximum_allowed_violations_by_impact: dict[str, int] | None = NoneThis change addresses the warning about implicit Optional types and makes the parameter typing more explicit.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def check_accessibility( | |
| self, page: Page, maximum_allowed_violations_by_impact: Dict[str, int] = None | |
| self, page: Page, maximum_allowed_violations_by_impact: dict[str, int] = None | |
| def check_accessibility( | |
| self, page: Page, maximum_allowed_violations_by_impact: dict[str, int] | None = None |
🧰 Tools
🪛 Ruff (0.8.2)
14-14: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
pages/login_page.py
Outdated
| @@ -19,7 +18,7 @@ def __init__(self, page: Page): | |||
| self.error_message = page.get_by_test_id("error") | |||
|
|
|||
| @allure.step("Login with username {username} and password {password}") | |||
| def login(self, username: Union[User, str], password: str): | |||
| def login(self, username: User | str, password: str): | |||
There was a problem hiding this comment.
Fix type annotation causing pipeline failure
The pipeline is failing with: TypeError: unsupported operand type(s) for |: 'module' and 'type'. This indicates either:
- The Python version in your environment doesn't support the pipe operator for unions (needs Python 3.10+), or
- There's an issue with how
Useris imported
Looking at the imports, there's a mismatch: you import with from enums import User but the enum is defined in enums/User.py.
Choose one of these options to fix it:
# Option 1: Revert to using typing.Union if you need Python <3.10 compatibility
- def login(self, username: User | str, password: str):
+ from typing import Union
+ def login(self, username: Union[User, str], password: str):# Option 2: Fix the import if User is a class in enums/User.py
- from enums import User
+ from enums.User import UserAdditionally, consider addressing these static analysis recommendations:
- Add a return type annotation:
-> None - Add a docstring describing what the method does
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def login(self, username: User | str, password: str): | |
| # Before (original import and function definition) | |
| # from enums import User | |
| # ... | |
| # def login(self, username: User | str, password: str): | |
| # After (updated import and function definition) | |
| from enums.User import User | |
| class LoginPage: | |
| # ... other methods and class content ... | |
| def login(self, username: User | str, password: str) -> None: | |
| """Logs in a user with the provided username and password. | |
| Args: | |
| username (User | str): A User instance or username as a string. | |
| password (str): The user's password. | |
| """ | |
| # Implementation goes here |
🧰 Tools
🪛 Ruff (0.8.2)
21-21: Missing return type annotation for public function login
Add return type annotation: None
(ANN201)
21-21: Missing docstring in public method
(D102)
🪛 GitHub Actions: Pre merge test
[error] 21-21: TypeError: unsupported operand type(s) for |: 'module' and 'type'
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/conftest.py (3)
28-28: Ensure consistent docstring formattingThis change improves docstring formatting for the
gotofixture. While this is a minor update, maintaining consistent docstring formatting across the codebase improves readability.
58-67: Improved docstring formatting and fixed duplicate 'Returns' sectionThe docstring has been reformatted for better readability. Note that there are still duplicate "Returns:" sections in this docstring (lines 59-61 and 65-67), which should be consolidated.
Returns: dict: Updated browser context arguments. See Also: https://playwright.dev/python/docs/api/class-browser#browser-new-contex -Returns: - dict: Updated browser context arguments.
50-50: Consider removing redundant 'scope' argumentThe static analyzer suggests removing
scope='function'as it's the default value for pytest fixtures and therefore redundant. However, keeping it explicit may help with readability for developers new to pytest.According to Ruff (PT003), you can simplify this to:
-@pytest.fixture(scope="function") +@pytest.fixture()🧰 Tools
🪛 Ruff (0.8.2)
50-50:
scope='function'is implied in@pytest.fixture()Remove implied
scopeargument(PT003)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/conftest.py(7 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/conftest.py (1)
utilities/axe_helper.py (1)
AxeHelper(9-49)
🪛 Ruff (0.8.2)
tests/conftest.py
50-50: scope='function' is implied in @pytest.fixture()
Remove implied scope argument
(PT003)
🔇 Additional comments (7)
tests/conftest.py (7)
51-51: Updated type annotations and added return typeThe type annotation has been updated from
Dicttodictand a return type annotation has been added. This change follows modern Python typing conventions where the built-indicttype is preferred overDictfrom the typing module.
89-89: Updated type annotations and added return typeThe type annotation has been updated from
Dicttodictand a return type annotation has been added. This change follows modern Python typing conventions where the built-indicttype is preferred overDictfrom the typing module.
107-107: Improved docstring formattingDocstring formatting has been updated for better readability, which is a good practice for code maintenance.
118-118: Improved docstring formattingDocstring formatting has been updated for the
get_public_ipfunction for better readability.
135-135: Improved docstring formattingDocstring formatting has been updated for the
attach_playwright_resultsfixture for better readability.
165-165: Improved docstring formattingDocstring formatting has been updated for the
pytest_runtest_makereporthook for better readability.
45-47:❓ Verification inconclusive
Changed fixture behavior from yield to return
The
axe_playwrightfixture has been changed from usingyieldto usingreturn. This is a significant behavioral change:
- Using
yieldallows for teardown operations after the test completes- Using
returndoesn't provide teardown functionalityEnsure this change is intentional, as it means no teardown operations will be performed for the AxeHelper instance.
🏁 Script executed:
#!/bin/bash # Check if any teardown operations might be needed for the AxeHelper instance rg -p "AxeHelper.*\.(close|cleanup|dispose|exit|teardown)" --no-headingLength of output: 73
I've prepared an updated review comment asking for explicit confirmation that the fixture's new behavior (directly returning the AxeHelper instance without a teardown phase) is truly what you want, especially since our searches so far haven't revealed any built-in teardown methods for AxeHelper. However, given that the initial regex didn't produce output—and to obtain a higher-confidence answer—we need a more targeted search to check for any teardown functionality in AxeHelper. For example:
Based on the results of these additional checks, please verify that no teardown (cleanup) methods are expected for AxeHelper. If the absence of such methods is intentional, it means that switching from
yieldtoreturnis acceptable. Otherwise, consider reintroducing an explicit teardown if resources need to be cleaned up.Rewritten Review Comment:
Action Required: Confirm Fixture Teardown Behavior for AxeHelper
The
axe_playwrightfixture now returns the AxeHelper instance directly instead of usingyield, which previously enabled execution of teardown operations after tests completed. Our initial search did not reveal any teardown-related methods (e.g.,close,cleanup,dispose,exit,teardown, or__exit__) in the AxeHelper implementation. Please double-check using the additional search script above that no cleanup functionality is defined or required for AxeHelper. If the lack of teardown is intentional given your resource management strategy, please confirm this decision. Otherwise, consider reinstituting a teardown mechanism if cleanup is necessary.
🏁 Script executed:
#!/bin/bash # Locate where AxeHelper is defined. rg -n "class AxeHelper" --no-heading # In the vicinity of AxeHelper’s definition, check for common teardown/cleanup method names. rg -n "class AxeHelper" -A 20 --no-heading | rg -n "def (close|cleanup|dispose|exit|teardown|__exit__)" --no-headingLength of output: 322
Confirm AxeHelper Teardown Behavior
The
axe_playwrightfixture now returns the AxeHelper instance directly instead of using ayieldthat could have executed any teardown operations. Our investigation of the AxeHelper implementation inutilities/axe_helper.pydid not reveal any methods related to teardown/cleanup (such asclose,cleanup,dispose,exit,teardown, or__exit__).
- If no cleanup is required for the AxeHelper instance, this behavior is acceptable.
- If resource cleanup is needed, consider reverting to the use of
yieldor explicitly handling teardown operations.Please confirm that the absence of any teardown logic is intentional.
Description
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
Documentation
Chores