Skip to content

[py] Fix pytest_ignore_collect hook to respect --ignore#15787

Merged
cgoldberg merged 1 commit intoSeleniumHQ:trunkfrom
mgorny:py-fix-ignore-hook
May 24, 2025
Merged

[py] Fix pytest_ignore_collect hook to respect --ignore#15787
cgoldberg merged 1 commit intoSeleniumHQ:trunkfrom
mgorny:py-fix-ignore-hook

Conversation

@mgorny
Copy link
Contributor

@mgorny mgorny commented May 24, 2025 •
edited by qodo-code-review bot
Loading

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.
    •

    @mgorny
    [py] Fix pytest_ignore_collect hook to respect --ignore
    d4efc1b 
    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-code-review
    Copy link
    Contributor

    qodo-code-review bot commented May 24, 2025

    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-code-review
    Copy link
    Contributor

    qodo-code-review bot commented May 24, 2025

    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

    cgoldberg
    cgoldberg reviewed May 24, 2025
    View reviewed changes
    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 •
    edited
    Loading

    The failure in CI is unrelated to this change.

    cgoldberg
    cgoldberg approved these changes May 24, 2025
    View reviewed changes
    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!

    This was referenced Sep 12, 2025
    Closed
    Closed
    Merged
    This was referenced Sep 19, 2025
    Closed
    Closed
    Closed
    This was referenced Oct 1, 2025
    Closed
    Closed
    Closed
    @dependabot dependabot bot mentioned this pull request Oct 24, 2025
    Closed
    This was referenced Dec 21, 2025
    Open
    Closed
    Closed
    Closed
    This was referenced Jan 19, 2026
    Open
    Open
    Open
    Merged
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @cgoldberg cgoldberg cgoldberg approved these changes

    Assignees

    No one assigned

    Labels

    C-py Python Bindings Review effort 1/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    3 participants

    @mgorny @cgoldberg @selenium-ci