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.

Sign up for GitHub

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

Fix "ValueError: relative path can't be expressed as a file URI" #7764

Merged
merged 1 commit into from
Jan 9, 2024
Merged

Fix "ValueError: relative path can't be expressed as a file URI" #7764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions src/tribler/gui/tests/test_tribler_window.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
from unittest.mock import Mock, patch

import pytest
from PyQt5.QtWidgets import QFileDialog

from tribler.gui.tribler_window import TriblerWindow


# pylint: disable=redefined-outer-name
@pytest.fixture
Comment on lines +9 to +10
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion to adhere to our Pylint rules:

Suggested change
# pylint: disable=redefined-outer-name
@pytest.fixture
@pytest.fixture(name="tribler_window")
def fixture_tribler_window():

That aside: please make sure the tests pass and re-request my review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion. From my point of view, for this particular case (for test_*.py files), it is preferable to use the following construction:

Example 1

# pylint: disable=redefined-outer-name
@pytest.fixture
def fixture_name():
    ...

Instead of the suggestion by pylint:

Example 2

@pytest.fixture(name="fixture_name")
def fixture_fixture_name():
    ...

Pylint is aimed at helping developers but not to be blindly obeyed. In many cases, pylint does a great job. However, in this particular case (from my point of view), the suggestion doesn't protect us from any error and does not make the code better. Instead, it makes the code more polluted as it requires unnecessary syntax complication.

I tried both approaches with Tribler, and based on my experience, Example 1 is as correct as Example 2, but it looks better and requires less time to write and correct, which saves developers' time.

Copy link
Contributor

Choose a reason for hiding this comment

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

In some cases, linters produce style violations, which are a matter of taste. However - in this case - Pylint correctly flags name shadowing. You can find more information in the pytest documentation: https://docs.pytest.org/en/stable/reference/reference.html#pytest-fixture (along with other methods to avoid name shadowing).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, but in this case, name shadowing is not a developer's mistake but a way how fixtures are implemented in pytest. There is nothing wrong with this fixture definition.

@pytest.fixture
def fixture_name():
    ...

Related:

Copy link
Contributor

Choose a reason for hiding this comment

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

That is indeed the way it is implemented in pytest and I'm not saying that it is the developer's mistake or fault (though I guess a developer can be faulted for not reading the docs to some extent). For example, pytest also rewrites the source code of test files and reinjects them into sys.modules, which is also dubious practice (and one we should not repeat).

There is nothing wrong with this fixture definition.

Agreed in general, disagreed for the fixture definition in this PR. The pytest docs themselves tell you what is wrong:

If a fixture is used in the same module in which it is defined, the function name of the fixture will be shadowed by the function arg that requests the fixture; [...]

Now pytest may be wrong about pytest but pytest seems like a trustworthy source for pytest information to me.

Just to repeat - myself and the docs - adding a "name" is not the only way to resolve this. Secondarily, this has nothing to do with Pylint, other than that it will also tell you the same thing. I like the idea of adding pylint-pytest though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@egbertbouman @xoriole please, share your opinion 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@qstokkink, no-no, I think I understand you. But in test test_2, there is no shadowing.

And yes, someone in the future could introduce mistakes (arithmetic, logical, or even related to shadowing) to test_2, which has nothing in common with the shadowing mentioned in the original pylint warning.

Copy link
Contributor

Choose a reason for hiding this comment

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

@drew2a Judging from your comment, I still do not think that you do understand. There is one specific type of situation that can happen solely due to the construction that causes name shadowing (in test_1). Not multiple as you just said. I gave an instance of this type of situation in the example above (#7764 (comment)).

The shadowing warning you get for test_1 causes trouble for test_2, which will not get a warning. It (test_2) will pass, even though it can and should fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@qstokkink, with all due respect, it's not shadowing that causes the potential problem; it's just a badly written test that caused the problem.

Here's a slightly changed example that "has problems due to shadowing." It still has them, even without any shadowing:

def something():
    return True


def test_2():
    assert something

Copy link
Contributor

Choose a reason for hiding this comment

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

@drew2a Yes, thank you, that is indeed the core issue: if something is now made into a fixture, which uses a name argument, exactly this type of error cannot happen.

def tribler_window():
""" Create mocked TriblerWindow instance"""
with patch('tribler.gui.tribler_window.TriblerWindow.__init__', Mock(return_value=None)):
window = TriblerWindow(Mock(), Mock(), Mock(), Mock())
window.pending_uri_requests = []
return window


def test_on_add_torrent_browse_file(tribler_window: TriblerWindow):
""" Test that the on_add_torrent_browse_file method works correctly"""
tribler_window.raise_window = Mock()
tribler_window.process_uri_request = Mock()

with patch.object(QFileDialog, 'getOpenFileNames', Mock(return_value=['.'])) as patched_getOpenFileNames:
tribler_window.on_add_torrent_browse_file()

assert tribler_window.raise_window.called
assert patched_getOpenFileNames.called
assert tribler_window.process_uri_request.called
15 changes: 8 additions & 7 deletions src/tribler/gui/tribler_window.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,15 +83,13 @@
from tribler.gui.utilities import (
connect,
create_api_key,
disconnect,
format_api_key,
get_font_path,
get_gui_setting,
get_image_path,
get_ui_file_path,
is_dir_writable,
set_api_key,
show_message_box,
tr,
)
from tribler.gui.widgets.instanttooltipstyle import InstantTooltipStyle
Expand Down Expand Up @@ -681,13 +679,16 @@ def on_create_torrent_updates(self, update_dict):

def on_add_torrent_browse_file(self, *_):
self.raise_window() # For the case when the action is triggered by tray icon
filenames = QFileDialog.getOpenFileNames(
filenames, *_ = QFileDialog.getOpenFileNames(
self, tr("Please select the .torrent file"), QDir.homePath(), tr("Torrent files%s") % " (*.torrent)"
)
if len(filenames[0]) > 0:
for filename in filenames[0]:
self.pending_uri_requests.append(Path(filename).as_uri())
self.process_uri_request()
if not filenames:
return

for filename in filenames:
uri = Path(filename).resolve().as_uri()
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any harm in using resolve() here but I wonder if this will fix the issue since the expected file is .torrent files which should exist as it is selected using the file dialog. The reproduction using ['.'] might not be representative enough for this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing the test from the original to the following produces the same reproducibility.

def test_on_add_torrent_browse_file(tribler_window: TriblerWindow):
    """ Test that the on_add_torrent_browse_file method works correctly"""
    tribler_window.raise_window = Mock()
    tribler_window.process_uri_request = Mock()

    with patch.object(QFileDialog, 'getOpenFileNames', Mock(return_value=['./any.torrent'])) as patched_getOpenFileNames:
        tribler_window.on_add_torrent_browse_file()

    assert tribler_window.raise_window.called
    assert patched_getOpenFileNames.called
    assert tribler_window.process_uri_request.called

self.pending_uri_requests.append(uri)
self.process_uri_request()

def start_download_from_uri(self, uri):
uri = uri.decode('utf-8') if isinstance(uri, bytes) else uri
Expand Down
Loading