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

[drew2a]

This PR adds a new test case to the test_tribler_window.py file. The test checks that the on_add_torrent_browse_file method in the TriblerWindow class works correctly. It mocks certain methods and uses patching to simulate user interaction with a file dialog. The test verifies that the appropriate methods are called and asserts their expected behavior.

The code changes in tribler_window.py modify the implementation of the on_add_torrent_browse_file method. It now handles cases where no filenames are selected from the file dialog, preventing unnecessary processing. Additionally, it resolves each filename to its absolute URI path before appending it to the list of pending URI requests.

These changes improve the functionality and reliability of adding torrent files through browsing in TriblerWindow.

Fixes #6345

[qstokkink]

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.

[drew2a]

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.

[qstokkink]

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

[drew2a]

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:

• False Positive W0621: Redefining name %r from outer scope when using pytest fixtures pylint-dev/pylint#6531
• https://github.com/pylint-dev/pylint-pytest

[qstokkink]

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.

[drew2a]

@egbertbouman @xoriole please, share your opinion 🙏

[drew2a]

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

[qstokkink]

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

[drew2a]

@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

[qstokkink]

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

[xoriole]

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.

[drew2a]

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

[qstokkink]

This PR has gotten stuck because we started debating the style rules enforced by our PR checks and we have no resolution yet. I want to get the PR unstuck by deferring the debate.

Please stick to our currently enforced rules in this PR (i.e., do not suppress redefined-outer-name) because they are the currently enforced rules.

Assuming you still believe we should change these rules, please open an issue so we can have a focused discussion on changing the style rules. If the team votes to adopt the new style rules, you can come in and change the code back and I will approve that PR without question.

[drew2a]

@qstokkink sure.

Please stick to our currently enforced rules in this PR (i.e., do not suppress redefined-outer-name) because they are the currently enforced rules.

We don't have an agreement that a developer should not suppress any linter warnings. The linter is just a tool that helps developers write good code and prevent bugs. The final decision should be with the developer (and the reviewer).

This PR increases the general system health as it fixes a bug. Even if it were written imperfectly (from a reviewer's perspective), it should be approved (at least, I would approve it even if I don't like the style, as Tribler is in better shape with this change than without it).

However, it is your right as a reviewer to request changes, and I appreciate it.

[synctext]

This is lengthy discussion on tomato versus tomato. Suppression of lint warning for code readability (another entity for the fixture). Hopefully we can improve our productivity and get more clarity on PR style. For instance, the pragmatic pull request guidelines by Google. Or force more 3rd developer mediation.

Let's try be pragmatic and involve another 3rd developer when PR author and reviewer are not in agreement on style matter or code rules. We could make it an automated Github rule to support this process.

So the broad policy of the team: approve if it improves. ?!?

[xoriole]

Since both approaches are used in the codebase. I'll approve the PR as a second reviewer.

[drew2a]

@qstokkink, we've discussed this PR for an hour at the most recent dev meeting. For this one, we had a consensus that the linter issues are less important than the change itself. Both forms of fixture definition are acceptable.

We can recap our debates from the dev meeting on Thursday if you need more clarity and want to hear opinions from other developers.

For cases like this, we decided to introduce the 'Tie Breaker' mechanism which will follow: in the case of disagreement between the PR author and a reviewer, the PR author or reviewer could ask for a second opinion from a randomly chosen developer (tie breaker); the decision from the tie breaker is the final decision in the dispute, and both parties should adhere to it.

I'll provide the automation for the tie breaker soon.

[qstokkink]

Even if it were written imperfectly [...]

Suppression of lint warning for code readability [...]

There is a misconception here: this issue here is the suppression of a Warning-level linting error. This is just below the Fatal/Error category that would instantly crash your code. It is in the category that causes long-term degradation of your code base. If it were just readabillity, it would be in the Convention/Refactor/Information classes.

I cannot support adding technical debt and piling on more technical debt because we already have it, is something I cannot support either.

That said, because another reviewer is now staking their reputation on approving this, I'll dismiss my own review and let it pass.

Overruled by second opinion.

[drew2a]

@qstokkink thank you for your understanding.

I don't want to start again about that "there is nothing wrong with suppressing a warning-level linting error for pylint fixtures". All our arguments are in the discussion above.

But I want to clarify that this PR does not have any technical debt, as it does not consist of any parts that should be rewritten in the future:

In software development, or any other IT field (e.g., Infrastructure, Networking, etc.) technical debt (also known as design debt[1] or code debt) is the implied cost of future reworking required when choosing an easy but limited solution instead of a better approach that could take more time.

I wanted to give an example of a Tribler PR that has technical debt, but instead, I'll finish with the funny note that

• Proposal: Introduce [Prototype] Tag for Experimental PRs #7799

almost justifies having technical debt as so-called "prototypes" almost repeat the definition of technical debt from above.

[qstokkink]

Your Wikipedia link lists a few causes of technical debt that I think are applicable here:

• Lack of process or understanding, where businesses are blind to the concept of technical debt, and make decisions without considering the implications.
• Lack of alignment to standards, where industry standard features, frameworks, and technologies are ignored. Eventually integration with standards will come and doing so sooner will cost less (similar to "delayed refactoring").[8]: 7 
• Lack of knowledge, when the developer doesn't know how to write elegant code.[9]

I could also link you to quite a few blogposts that will try to convince you that ignoring linter warnings is a form of technical debt. However, that only counts as anecdotal evidence of random people from the Internet.

That said, I agree with you that if nobody ever changes the code, it does not count as technical debt. However, I do predict that this code will change. Of course, this is only mostly based on experience. Nobody can predict the future.

Also, completely agreed that prototypes are - by definition - technical debt. No discussion there. I do consider them a necessary part of code base evolution though.

Lastly, also agreed that we are beating a dead horse. I did not want to start another discussion. I just wanted to be transparent on why I will not approve this PR even after a second opinion, for posterity: just so I could look back later and say "I told you so".