Skip to content
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

[py] moved all pytest configuration settings from pytest.ini file to pyproject.toml #14256

Merged
merged 7 commits into from
Aug 22, 2024

Conversation

sandeepsuryaprasad
Copy link
Contributor

@sandeepsuryaprasad sandeepsuryaprasad commented Jul 12, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Moved all Pytest settings from pytest.ini file to pyproject.toml

Motivation and Context

This avoids having to maintain a separate config file for pytest settings. All pytest configuration settings can be maintained in pyproject.toml

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement, Configuration changes


Description

  • Migrated all pytest configuration settings from pytest.ini to pyproject.toml.
  • Updated Bazel build configuration to reference pyproject.toml instead of pytest.ini.
  • Removed the pytest.ini file as it is no longer needed.

Changes walkthrough 📝

Relevant files
Configuration changes
BUILD.bazel
Update Bazel build configuration to use `pyproject.toml` 

py/BUILD.bazel

  • Replaced pytest.ini with pyproject.toml in the data section.
+1/-1     
pytest.ini
Remove `pytest.ini` file                                                                 

py/pytest.ini

  • Removed the pytest.ini file and its contents.
+0/-15   
Enhancement
pyproject.toml
Migrate pytest settings to `pyproject.toml`                           

py/pyproject.toml

  • Added pytest configuration settings under [tool.pytest.ini_options].
  • Included markers for various test expectations.
  • +16/-0   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Verify and ensure that pyproject.toml is included in all relevant Bazel build targets

    Ensure that the newly added pyproject.toml file is correctly referenced in all
    relevant Bazel build targets to maintain consistency across the build configuration.

    py/BUILD.bazel [335]

     data = [
    -    "pyproject.toml",
    +    "pyproject.toml",  # Ensure this file is included in all relevant build targets
         "setup.cfg",
         ...
     ]
     
    Suggestion importance[1-10]: 9

    Why: Ensuring that pyproject.toml is included in all relevant build targets is crucial for maintaining consistency across the build configuration, making it a high-priority best practice.

    9
    Add a comment specifying the default value for log_cli to ensure clarity and prevent configuration errors

    It is recommended to add a default value for log_cli to ensure consistent behavior
    across different environments or if the configuration is accidentally omitted.

    py/pyproject.toml [8]

    -log_cli = true
    +log_cli = true  # Default: false
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a comment about the default value for log_cli enhances clarity and helps prevent configuration errors, making it a valuable best practice.

    8
    Maintainability
    Improve the readability and maintainability of pytest markers by renaming them to more descriptive names

    Consider using a more descriptive marker name for the xfail markers to indicate that
    these are browser-specific expected failures. This can improve readability and
    maintainability of the test configurations.

    py/pyproject.toml [10-17]

     markers = [
    -    "xfail_chrome: Tests expected to fail in Chrome",
    -    "xfail_edge: Tests expected to fail in Edge",
    +    "expected_failure_chrome: Tests expected to fail in Chrome",
    +    "expected_failure_edge: Tests expected to fail in Edge",
         ...
     ]
     
    Suggestion importance[1-10]: 7

    Why: The suggestion to use more descriptive marker names improves readability and maintainability, but it is not crucial for functionality. It is a good practice for better code clarity.

    7
    Enhancement
    Group xfail markers under a common subsection to enhance configuration readability

    Consider grouping the xfail markers under a common subsection to enhance the
    organization and readability of the pytest configuration.

    py/pyproject.toml [10-17]

     markers = [
    -    "xfail_chrome: Tests expected to fail in Chrome",
    -    "xfail_edge: Tests expected to fail in Edge",
    -    ...
    +    "browser_failures: {
    +        xfail_chrome: Tests expected to fail in Chrome",
    +        xfail_edge: Tests expected to fail in Edge",
    +        ...
    +    }"
     ]
     
    Suggestion importance[1-10]: 6

    Why: Grouping xfail markers under a common subsection can improve readability, but the suggested code structure is not standard and may introduce confusion. The improvement is minor.

    6

    @diemol
    Copy link
    Member

    diemol commented Jul 29, 2024

    @sandeepsuryaprasad
    Copy link
    Contributor Author

    @diemol is there any action pending from my side regarding this PR.

    @diemol
    Copy link
    Member

    diemol commented Aug 9, 2024

    Yes, my question above. It does seem not all tests were executed. Can you please verify?

    @sandeepsuryaprasad
    Copy link
    Contributor Author

    Yes, my question above. It does seem not all tests were executed. Can you please verify?

    All Python tests are passed. But there are some errors from .NET modules. I think this has got nothing to do with the changes that I have made.

    (08:44:24) ERROR: Traceback (most recent call last):
    	File "/home/runner/.bazel/external/rules_dotnet~/dotnet/private/transitions/tfm_transition.bzl", line 59, column 13, in _impl
    		fail("Label {0} does not support the target framework: {1}".format(attr.name, incoming_tfm))
    Error in fail: Label fixtures does not support the target framework: net7.0
    (08:44:24) ERROR: /home/runner/work/selenium/selenium/dotnet/test/common/BUILD.bazel:35:15: Errors encountered while applying Starlark transition
    (08:44:24) ERROR: /home/runner/work/selenium/selenium/dotnet/src/webdriver/BUILD.bazel: no such target '//dotnet/src/webdriver:webdriver': target 'webdriver' not declared in package 'dotnet/src/webdriver' defined by /home/runner/work/selenium/selenium/dotnet/src/webdriver/BUILD.bazel (did you mean WebDriver.cs, or IWebDriver.cs?)
    
    
    (08:45:21) ERROR: /home/runner/work/selenium/selenium/dotnet/test/support/UI/BUILD.bazel:12:24: no such target '//dotnet/src/webdriver:webdriver': target 'webdriver' not declared in package 'dotnet/src/webdriver' defined by /home/runner/work/selenium/selenium/dotnet/src/webdriver/BUILD.bazel (did you mean WebDriver.cs, or IWebDriver.cs?) and referenced by '//dotnet/test/support/UI:SelectTests'
    
    

    @diemol
    Copy link
    Member

    diemol commented Aug 9, 2024

    This execution has around 315 python tests executed (you can search with //py:common and //py:unit). Whereas in this execution I don't see any //py:common tests executed.

    @sandeepsuryaprasad
    Copy link
    Contributor Author

    This execution has around 315 python tests executed (you can search with //py:common and //py:unit). Whereas in this execution I don't see any //py:common tests executed.

    Can you please re-trigger the workflow.

    @sandeepsuryaprasad
    Copy link
    Contributor Author

    @diemol I just verified the results. All the tests have been executed and passed.

    @sandeepsuryaprasad
    Copy link
    Contributor Author

    sandeepsuryaprasad commented Aug 20, 2024

    @diemol all test's are passing.. could you check on this one please.

    @diemol
    Copy link
    Member

    diemol commented Aug 20, 2024

    Seems to be fine. I just want @AutomatedTester to have a look before merging.

    @AutomatedTester
    Copy link
    Member

    Thanks for the PR!

    @AutomatedTester AutomatedTester merged commit 26f9aa2 into SeleniumHQ:trunk Aug 22, 2024
    13 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants