Skip to content

Conversation

@mgorny
Copy link
Contributor

@mgorny mgorny commented May 24, 2025

User description

🔗 Related Issues

💥 What does this PR do?

Fix the pytest_ignore_collect hook to respect --ignore specified by the user.

🔧 Implementation Notes

Returning False stops pytest from consulting additional hooks, including its default hooks that are necessary to process --ignore option. By returning True or None, the hook combines files ignored by default with ignores specified by the user.

💡 Additional Considerations

This is a pure test change.

🔄 Types of changes

  • Bug fix (backwards compatible)

PR Type

Bug fix, Tests


Description

  • Fixes pytest_ignore_collect to respect --ignore option

  • Ensures default pytest ignores are combined with custom ignores

  • Prevents premature stopping of hook processing


Changes walkthrough 📝

Relevant files
Bug fix
conftest.py
Fix test collection ignore logic for pytest                           

py/conftest.py

  • Modified pytest_ignore_collect to return True or None instead of False
  • Ensures compatibility with pytest's --ignore option
  • Prevents stopping further hook processing by pytest
  • +3/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Fix the `pytest_ignore_collect` hook to respect `--ignore` specified by the user. Returning `False` stops pytest from consulting additional hooks, including its default hooks that are necessary to process `--ignore` option. By returning `True` or `None`, the hook combines files ignored by default with ignores specified by the user.
    @selenium-ci selenium-ci added the C-py Python Bindings label May 24, 2025
    @qodo-merge-pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Return Value Logic

    The hook now returns True for ignored paths and None otherwise. Consider if this matches pytest's expected behavior, as the documentation suggests returning True to ignore and False to not ignore.

    if len([d for d in _drivers if d.lower() in collection_path.parts]) > 0: return True return None
    @qodo-merge-pro
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Optimize iteration pattern

    Use a more efficient approach by returning early when a match is found, rather
    than building a full list first. This avoids unnecessary iterations through all
    drivers when a match is found early.

    py/conftest.py [93-95]

    -if len([d for d in _drivers if d.lower() in collection_path.parts]) > 0: - return True +for d in _drivers: + if d.lower() in collection_path.parts: + return True return None
    • Apply / Chat
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion improves efficiency by returning early upon finding a match, which can reduce unnecessary iterations. The logic is equivalent and correct, but the impact is moderate since the original code is already functional.

    Low
    • More
    Copy link
    Member

    @cgoldberg cgoldberg left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    @mgorny this looks fine, but can you give an example of how this was broken before? (just so I understand the change better)

    @mgorny
    Copy link
    Contributor Author

    mgorny commented May 24, 2025

    For example, if you run:

    pytest test/ --ignore test/selenium/webdriver/common/bidi_webextension_tests.py 

    Without the patch, test/selenium/webdriver/common/bidi_webextension_tests.py will be collected anyway. With the patch, it will be correctly ignored.

    @cgoldberg
    Copy link
    Member

    cgoldberg commented May 24, 2025

    The failure in CI is unrelated to this change.

    Copy link
    Member

    @cgoldberg cgoldberg left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    got it. thanks for the contribution!

    @cgoldberg cgoldberg merged commit 793d99d into SeleniumHQ:trunk May 24, 2025
    18 of 19 checks passed
    @mgorny mgorny deleted the py-fix-ignore-hook branch May 24, 2025 15:45
    @mgorny
    Copy link
    Contributor Author

    mgorny commented May 24, 2025

    Thanks!

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    3 participants