-
Notifications
You must be signed in to change notification settings - Fork 451
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
Replace lambdas with function calls #7466
Replace lambdas with function calls #7466
Conversation
This pull request is instead of #7441 which I messed up. |
I didn't see the test result for "codecov/patch", so I thought it was fine to convert to "ready for review." Now I see I still need to fix it, so I converted it back to "draft". Edit: My mistake. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the tremendous effort you've put into this work. The quality of the Tribler codebase has improved with these changes. However, I have some comments which I've added for your consideration.
src/tribler/core/components/gigachannel/community/gigachannel_community.py
Outdated
Show resolved
Hide resolved
src/tribler/core/components/gigachannel/community/gigachannel_community.py
Outdated
Show resolved
Hide resolved
src/tribler/core/components/libtorrent/download_manager/download_manager.py
Outdated
Show resolved
Hide resolved
src/tribler/core/components/libtorrent/tests/test_download_manager.py
Outdated
Show resolved
Hide resolved
src/tribler/core/components/libtorrent/tests/test_download_manager.py
Outdated
Show resolved
Hide resolved
src/tribler/core/components/libtorrent/tests/test_download_manager.py
Outdated
Show resolved
Hide resolved
src/tribler/core/components/tunnel/community/tunnel_community.py
Outdated
Show resolved
Hide resolved
When this PR will be ready to be accepted, I will of course rewrite commit messages and squash accordingly. Right now it's a work in progress so it's a mess. |
Edit: Never mind, it's fine now. The latest rebasing from |
Edit 3: I ran it again by changing them to |
This came from a recent pull request: def test_start_download_existing_download(fake_dlmgr):
"""
Testing the addition of a torrent to the libtorrent manager, if there is a pre-existing download.
"""
infohash = b'a' * 20
mock_download = MagicMock(
get_def=MagicMock(return_value=MagicMock(get_trackers=set))
)
mock_ltsession = MagicMock()
fake_dlmgr.downloads[infohash] = mock_download
fake_dlmgr.get_session = lambda *_: mock_ltsession
download = await fake_dlmgr.start_download(tdef=TorrentDefNoMetainfo(infohash, 'name'), checkpoint_disabled=True)
assert download == mock_download
fake_dlmgr.downloads.clear() Notice there's an It's in |
The Application Tester is failing for some reason: https://github.com/Tribler/tribler/actions/runs/7049092932/job/19186780929?pr=7466 See: #7739 I'll fix it soon. |
@Solomon1732, the Application Tester has been fixed. Please, rebase your PR into the main branch. |
@Solomon1732 I see that this PR has been open for quite a while now. Do you need any help in getting this PR ready? |
@qstokkink I admit I do. I don't know how to cover the following warnings in
You can see they're all code changes which are internal, and don't necessarily affect a function's output. I don't know how to approach them. If I can get ideas or pointers for how to write tests for these failures, I would be happy. |
Ah yes, those are pretty nasty. I could cover most of them (except the Click me for changes!diff --git a/src/tribler/core/components/libtorrent/tests/test_download_manager.py b/src/tribler/core/components/libtorrent/tests/test_download_manager.py
index 493ee66eb..e3b1a598b 100644
--- a/src/tribler/core/components/libtorrent/tests/test_download_manager.py
+++ b/src/tribler/core/components/libtorrent/tests/test_download_manager.py
@@ -1,6 +1,7 @@
import asyncio
from asyncio import Future, gather, get_event_loop, sleep
-from unittest.mock import MagicMock
+from functools import partial
+from unittest.mock import MagicMock, Mock
from unittest.mock import AsyncMock
import pytest
@@ -491,3 +492,71 @@ async def test_check_for_dht_ready(fake_dlmgr):
fake_dlmgr.get_session().status().dht_nodes = 1000
# If the session has enough peers, it should finish instantly
await fake_dlmgr._check_dht_ready()
+
+
+def test_update_trackers(fake_dlmgr: DownloadManager) -> None:
+ fake_download, dl_state = create_fake_download_and_state()
+ fake_dlmgr.downloads[fake_download.infohash] = fake_download
+ fake_metainfo = {b'info': {b'name': b"test_download"}}
+ fake_download.get_def().metainfo = fake_metainfo
+
+ fake_tracker1 = "127.0.0.1/test-announce1"
+
+ fake_dlmgr.update_trackers(fake_download.infohash, [fake_tracker1])
+
+ assert fake_metainfo["announce"] == fake_tracker1
+ assert "announce-list" not in fake_metainfo
+
+
+def test_update_trackers_list(fake_dlmgr: DownloadManager) -> None:
+ fake_download, dl_state = create_fake_download_and_state()
+ fake_dlmgr.downloads[fake_download.infohash] = fake_download
+ fake_metainfo = {b'info': {b'name': b"test_download"}}
+ fake_download.get_def().metainfo = fake_metainfo
+
+ fake_tracker1 = "127.0.0.1/test-announce1"
+ fake_tracker2 = "127.0.0.1/test-announce2"
+
+ fake_dlmgr.update_trackers(fake_download.infohash, [fake_tracker1, fake_tracker2])
+
+ assert "announce" not in fake_metainfo
+ assert fake_metainfo["announce-list"] == [[fake_tracker1, fake_tracker2]]
+
+
+def test_update_trackers_list_append(fake_dlmgr: DownloadManager) -> None:
+ fake_download, dl_state = create_fake_download_and_state()
+ fake_dlmgr.downloads[fake_download.infohash] = fake_download
+ fake_metainfo = {b'info': {b'name': b"test_download"}}
+ fake_download.get_def().metainfo = fake_metainfo
+
+ fake_tracker1 = "127.0.0.1/test-announce1"
+ fake_tracker2 = "127.0.0.1/test-announce2"
+
+ fake_dlmgr.update_trackers(fake_download.infohash, [fake_tracker1])
+ fake_download.get_def().get_tracker = Mock(return_value=fake_tracker1)
+ fake_dlmgr.update_trackers(fake_download.infohash, [fake_tracker2])
+
+ assert fake_metainfo["announce"] == fake_tracker1
+ assert fake_metainfo["announce-list"] == [[fake_tracker1, fake_tracker2]]
+
+
+def test_get_download_rate_limit(fake_dlmgr: DownloadManager) -> None:
+ fake_value = 42
+ settings = {}
+ fake_dlmgr.get_session = Mock(return_value=Mock(get_settings=Mock(return_value=settings)))
+ fake_dlmgr.get_session().download_rate_limit = partial(settings.get, "download_rate_limit")
+ fake_dlmgr.ltsessions = {0: fake_dlmgr.get_session()}
+ fake_dlmgr.set_download_rate_limit(fake_value, 0)
+
+ assert fake_value == fake_dlmgr.get_download_rate_limit()
+
+
+def test_get_upload_rate_limit(fake_dlmgr: DownloadManager) -> None:
+ fake_value = 42
+ settings = {}
+ fake_dlmgr.get_session = Mock(return_value=Mock(get_settings=Mock(return_value=settings)))
+ fake_dlmgr.get_session().upload_rate_limit = partial(settings.get, "upload_rate_limit")
+ fake_dlmgr.ltsessions = {0: fake_dlmgr.get_session()}
+ fake_dlmgr.set_upload_rate_limit(fake_value, 0)
+
+ assert fake_value == fake_dlmgr.get_upload_rate_limit()
diff --git a/src/tribler/core/components/tunnel/tests/test_dispatcher.py b/src/tribler/core/components/tunnel/tests/test_dispatcher.py
index c31c5cea7..e068570a2 100644
--- a/src/tribler/core/components/tunnel/tests/test_dispatcher.py
+++ b/src/tribler/core/components/tunnel/tests/test_dispatcher.py
@@ -1,7 +1,8 @@
+import asyncio
from unittest.mock import Mock
import pytest
-from ipv8.messaging.anonymization.tunnel import CIRCUIT_STATE_EXTENDING, CIRCUIT_STATE_READY, CIRCUIT_TYPE_DATA
+from ipv8.messaging.anonymization.tunnel import CIRCUIT_STATE_EXTENDING, CIRCUIT_STATE_READY, CIRCUIT_TYPE_DATA, Circuit
from ipv8.util import succeed
from tribler.core.components.tunnel.community.dispatcher import TunnelDispatcher
@@ -127,3 +128,35 @@ def test_check_connections(dispatcher, mock_circuit):
assert connection not in dispatcher.con_to_cir
assert mock_circuit.circuit_id not in dispatcher.cid_to_con
assert 2 in dispatcher.cid_to_con
+
+
+async def test_on_data_after_select(dispatcher: TunnelDispatcher, mock_circuit: Circuit) -> None:
+ mock_connection = Mock(udp_connection=None)
+ mock_request = Mock(destination=("0.0.0.0", 1024), data=b'a')
+ dispatcher.set_socks_servers([mock_connection.socksserver])
+ dispatcher.tunnels.circuits = {}
+ dispatcher.tunnels.create_circuit = Mock(return_value=mock_circuit)
+ dispatcher.on_socks5_udp_data = Mock(return_value=None)
+ mock_circuit.ready = asyncio.Future()
+
+ mock_circuit.ready.set_result(True)
+ dispatcher.select_circuit(mock_connection, mock_request)
+ await asyncio.sleep(0)
+
+ assert dispatcher.on_socks5_udp_data.called
+
+
+async def test_on_data_after_select_no_result(dispatcher: TunnelDispatcher, mock_circuit: Circuit) -> None:
+ mock_connection = Mock(udp_connection=None)
+ mock_request = Mock(destination=("0.0.0.0", 1024), data=b'a')
+ dispatcher.set_socks_servers([mock_connection.socksserver])
+ dispatcher.tunnels.circuits = {}
+ dispatcher.tunnels.create_circuit = Mock(return_value=mock_circuit)
+ dispatcher.on_socks5_udp_data = Mock(return_value=None)
+ mock_circuit.ready = asyncio.Future()
+
+ mock_circuit.ready.set_result(None)
+ dispatcher.select_circuit(mock_connection, mock_request)
+ await asyncio.sleep(0)
+
+ assert not dispatcher.on_socks5_udp_data.called The "dummy mode" code seems like dead code to me (and this should not be in our production code at all IMO). |
@qstokkink Thanks a bunch! I'll work on adding your changes to my PR. I'll also remove |
@Solomon1732 No worries, I'm happy to help. I would advise against removing |
@Solomon1732 @qstokkink, as an option, we can ignore all uncovered lines related to @qstokkink, could you please create an issue regarding the potential removal of |
@drew2a will do as you advise. Thanks a bunch! Yeah, it looks like it'll make this PR a hairy one. |
src/tribler/core/components/libtorrent/tests/test_download_manager.py
Outdated
Show resolved
Hide resolved
I have no idea how to fix the errors "final newline missing" and "Unexpected line ending format. There is 'CRLF' while it should be 'LF'." I followed both Codasity's article Error caused by incompatible line endings and Github's page Configuring Git to handle line endings but to no avail. I can't find any CRLF ending in any python file in the repo of my PR. I even ran the following command as per Codasity's article above.: find . -type f -name '*.py' -exec file {} \; | grep "CR line" It found nothing. |
@Solomon1732 are you using PyCharm? If so, there is a sneaky setting in the bottom right: EDIT: If I'm suggesting fixes anyway:
@pytest.fixture(name="fake_dlmgr")
async def fixture_fake_dlmgr(tmp_path_factory): |
@qstokkink I Visual Studio Code. Or I use Geany if I find VSC too cumbersome for this or that task. |
@Solomon1732 IIRC VSC also has its line endings setting in the bottom-right corner somewhere. |
I finally fixed it somehow. I did an ugly hack of committing, squashing, and force pushing. It should look fine on the PR, though. |
👍 whatever works. It seems our Codacy check did not run (for some reason) now though. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! I've added some comments, most of which are related to coding style. All of them come with suggestions, so if you agree with them, you could just apply the working version.
src/tribler/core/components/libtorrent/download_manager/download_manager.py
Outdated
Show resolved
Hide resolved
src/tribler/core/components/libtorrent/download_manager/download_manager.py
Outdated
Show resolved
Hide resolved
src/tribler/core/components/libtorrent/download_manager/download_manager.py
Outdated
Show resolved
Hide resolved
src/tribler/core/components/libtorrent/download_manager/download_manager.py
Outdated
Show resolved
Hide resolved
src/tribler/core/components/libtorrent/tests/test_download_manager.py
Outdated
Show resolved
Hide resolved
src/tribler/core/components/libtorrent/tests/test_download_manager.py
Outdated
Show resolved
Hide resolved
src/tribler/core/components/libtorrent/tests/test_download_manager.py
Outdated
Show resolved
Hide resolved
src/tribler/core/components/libtorrent/tests/test_download_manager.py
Outdated
Show resolved
Hide resolved
src/tribler/core/components/libtorrent/tests/test_download_manager.py
Outdated
Show resolved
Hide resolved
src/tribler/core/components/libtorrent/tests/test_download_manager.py
Outdated
Show resolved
Hide resolved
@drew2a thanks! I applied all of your recommendations, so I marked all of them as "Resolved" instead of writing "done" and resolving each one. Edit: Minor phrasing fixes. |
I suspect Codasity doesn't understand Python's |
Don't go down this rabbit hole (see our discussion with @qstokkink #7764 (review)) :) Basically, you have two options:
I'll approve the PR in both cases, as both options are correct. However, @qstokkink most likely will not approve the PR if you suppress the warning. So, my advice is to opt for number 2 and follow @qstokkink's suggestion. |
src/tribler/core/components/libtorrent/tests/test_download_manager.py
Outdated
Show resolved
Hide resolved
@drew2a I looked at it, and I will do it 😰 . I indeed don't want to go into that rabbit hole. |
src/tribler/core/components/libtorrent/tests/test_download_manager.py
Outdated
Show resolved
Hide resolved
I'm not sure why this test failed, or if it's my PR's code or not. May I get some help? I think it may has to do with #7798?
Edit: Based on #7798 (comment) it may indeed has to do with it. |
It seems the following two lines can/should be removed:
tribler/scripts/exit_node/run_exit_node.py Line 204 in 959169e
|
@Solomon1732 @qstokkink, I'll fix it within a couple of hours. The fix: |
@Solomon1732 fixed. |
Replace lambdas with function calls. This was done either by extracting a lambda into a function or method, or by replacing it with a library function call. Minor code changes: The `u` prefix was removed from strings. Instead of using the `%` operator with strings or concatenating multiple of them, f-strings were used. Replaced complex one-like if-else chains with dicts and calls to `dict.get`. Instead of creating lists and adding them (which returns a new list), a list is created from the get-go by extending it via the star operator during its creation. Change according to code review In test_download_manager.py, I replaced lambdas with `MagicMock` and `AsyncMock` as appropriate. I also rearranged mock methods inside tests. In `dispatcher.py`, I rewrote lambda as an internal callback function. In `conf.py`, I extracted a lambda into a function. In `gigachannel_community.py`, I extracted two repeated lambdas into a single module-internal function.
src/tribler/core/components/libtorrent/tests/test_download_manager.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good except for one small mistake in the tests, which can be easily fixed. I proposed a complete solution during the previous review round, but it appears to have been overlooked.
I added tests. One is to test `convert_rate` and `reverse_converse_rate` in `download_manager.py`. Upstream removed code from `tribler/src/tribler/gui/dialogs/feedbackdialog.py`. Therefore, this commit removes said changes as well. I made `test_start_download_existing_download` in `src/tribler/core/components/libtorrent/tests/test_download_manager.py` async to fix accidental deletion of it. I added the following tests to `src/tribler/core/components/libtorrent/tests/test_download_manager.py`: 1. test_update_trackers 2. test_get_download_rate_limit 3. test_get_upload_rate_limit I added the following tests to `src/tribler/core/components/libtorrent/tests/test_dispatcher.py`: 1. test_on_data_after_select 2. test_on_data_after_select_no_result
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your patience.
It could be merged if @qstokkink or @kozlovsky don't have any objections 👍 . |
Replace lambdas with function calls. This was done either by extracting a lambda into a function or method, or by replacing it with a library function call. Also some minor code quality changes.
This PR fixes #7432