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

Ignore spurious ResourceWarning in tests when opening the rendered result in the browser on Linux #35

Merged
merged 1 commit into from
Jan 17, 2022

Conversation

martinRenou
Copy link
Collaborator

@martinRenou martinRenou commented Jan 13, 2022

This prevents the tests from failing when running them with the --open-browser option. As discussed in #33 (comment)

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

(Somehow, my first review didn't get picked up correctly, sorry)

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

The issue with this very broad brush approach is it also silences any ResourceWarning anywhere in the code under test (or the test suite), and regardless of whether `--open-browser`` is passed. Instead, what I recommend you do is

You can add the warnings filter to _open_browser in the open_browser fixture in conftest.py.

as this will ensure that it is only activated when it is needed (unless you can't get that to work, in which case there are still other less drastic options) Further, you should limit the filter to just the subprocess module (fourth field in the filter syntax, right after category).

Thanks!

Furthermore, I'd suggest limiting the filter to the subprocess module

@martinRenou
Copy link
Collaborator Author

Thanks for the review.

You can add the warnings filter to _open_browser in the open_browser fixture in conftest.py.

I fail to understand how this would look like? I tried multiple things, like:

@pytest.fixture
def open_browser(request):
    """Show the passed URL in the user's web browser if passed."""
    def _open_browser(url):
        if request.config.getoption(OPEN_BROWSER_OPTION):
            pytestmark = pytest.mark.filterwarnings("ignore::ResourceWarning")

            webbrowser.open_new_tab(url)
    return _open_browser

Or

@pytest.fixture
def open_browser(request):
    """Show the passed URL in the user's web browser if passed."""
    @pytest.mark.filterwarnings("ignore::ResourceWarning")
    def _open_browser(url):
        if request.config.getoption(OPEN_BROWSER_OPTION):
            webbrowser.open_new_tab(url)
    return _open_browser

But that does not work. I didn't find any documentation about filtering warnings on pytest fixtures, it seems to only be documented for test cases.

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Jan 14, 2022

Sorry, I meant just using the normal warnings.filterwarnings, i.e.

@pytest.fixture
def open_browser(request):
    """Show the passed URL in the user's web browser if passed."""
    def _open_browser(url):
        if request.config.getoption(OPEN_BROWSER_OPTION):
            warnings.filterwarnings(
                'ignore', category=ResourceWarning, module='subprocess.*')
            webbrowser.open_new_tab(url)
    return _open_browser

(You might need to play around with the module option; if you can't get it to work at all, you can elide it if nessesary)

This ensures it works regardless of where/what scope the warning fires at (which can be tricky due to relating to non-deterministic, OS-specific source/destructor behavior, as suggested by the nature of the warning and the traceback), and gets applied immediately before opening the browser, if and only if the browser is to be opened (and without having to add it directly to any test code that uses it).

@CAM-Gerlach CAM-Gerlach changed the title Ignore ResourceWarning Ignore spurious ResourceWarning in tests when opening the rendered result in the browser on Linux Jan 14, 2022
This fixes the test when using the --open-browser option on Linux
@martinRenou martinRenou force-pushed the ignore_resource_warning branch from 2ec0fdf to c14d485 Compare January 17, 2022 08:50
@martinRenou
Copy link
Collaborator Author

Oh I see, thanks!

I updated the PR

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @martinRenou !

@CAM-Gerlach CAM-Gerlach merged commit 2db3ff3 into spyder-ide:master Jan 17, 2022
@martinRenou martinRenou deleted the ignore_resource_warning branch January 17, 2022 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants