From 7dab8335e2f399ef9d9e0e43e39375fd36b15e4a Mon Sep 17 00:00:00 2001 From: Jimmy Date: Sun, 21 Jan 2018 15:37:22 +1300 Subject: [PATCH] Greasemonkey: handle downloads that complete fast When `@require`ing local files (with the `file://` scheme) the greasemonkey manager was not catching the DownloadItem.finished signal because it was being emitted before it had managed to connect. I didn't see this happening while testing with files that should have been in cache but I wouldn't be surprised. I had to change the download mock to be able to give it the appearance of asynchronicity. Now when using it one must set download.successful appropriately before firing download.finished. I also added a list of downloads to the stub so a test could enumerate them in case the unit-under-test didn't have a reference to them. --- qutebrowser/browser/greasemonkey.py | 9 ++++++--- tests/helpers/fixtures.py | 2 +- tests/helpers/stubs.py | 4 +++- tests/unit/browser/test_adblock.py | 14 +++++++++++--- 4 files changed, 21 insertions(+), 8 deletions(-) diff --git a/qutebrowser/browser/greasemonkey.py b/qutebrowser/browser/greasemonkey.py index d37340d88..9f7e26f53 100644 --- a/qutebrowser/browser/greasemonkey.py +++ b/qutebrowser/browser/greasemonkey.py @@ -287,9 +287,12 @@ class GreasemonkeyManager(QObject): auto_remove=True) download.requested_url = url self._in_progress_dls.append(download) - download.finished.connect( - functools.partial(self._on_required_download_finished, - script, download)) + if download.successful: + self._on_required_download_finished(script, download) + else: + download.finished.connect( + functools.partial(self._on_required_download_finished, + script, download)) def scripts_for(self, url): """Fetch scripts that are registered to run for url. diff --git a/tests/helpers/fixtures.py b/tests/helpers/fixtures.py index 7c9fc93ae..4ae9f125d 100644 --- a/tests/helpers/fixtures.py +++ b/tests/helpers/fixtures.py @@ -530,5 +530,5 @@ def download_stub(win_registry, tmpdir, stubs): """Register a FakeDownloadManager.""" stub = stubs.FakeDownloadManager(tmpdir) objreg.register('qtnetwork-download-manager', stub) - yield + yield stub objreg.delete('qtnetwork-download-manager') diff --git a/tests/helpers/stubs.py b/tests/helpers/stubs.py index 0167926bd..fbe7035e3 100644 --- a/tests/helpers/stubs.py +++ b/tests/helpers/stubs.py @@ -572,7 +572,7 @@ class FakeDownloadItem(QObject): super().__init__(parent) self.fileobj = fileobj self.name = name - self.successful = True + self.successful = False class FakeDownloadManager: @@ -581,6 +581,7 @@ class FakeDownloadManager: def __init__(self, tmpdir): self._tmpdir = tmpdir + self.downloads = [] @contextlib.contextmanager def _open_fileobj(self, target): @@ -603,4 +604,5 @@ class FakeDownloadManager: download_item = FakeDownloadItem(target.fileobj, name=url.path()) with (self._tmpdir / url.path()).open('rb') as fake_url_file: shutil.copyfileobj(fake_url_file, download_item.fileobj) + self.downloads.append(download_item) return download_item diff --git a/tests/unit/browser/test_adblock.py b/tests/unit/browser/test_adblock.py index e3b0e4576..5b353efb9 100644 --- a/tests/unit/browser/test_adblock.py +++ b/tests/unit/browser/test_adblock.py @@ -206,6 +206,7 @@ def test_disabled_blocking_update(basedir, config_stub, download_stub, while host_blocker._in_progress: current_download = host_blocker._in_progress[0] with caplog.at_level(logging.ERROR): + current_download.successful = True current_download.finished.emit() host_blocker.read_hosts() for str_url in URLS_TO_CHECK: @@ -221,6 +222,8 @@ def test_no_blocklist_update(config_stub, download_stub, host_blocker = adblock.HostBlocker() host_blocker.adblock_update() host_blocker.read_hosts() + for dl in download_stub.downloads: + dl.successful = True for str_url in URLS_TO_CHECK: assert not host_blocker.is_blocked(QUrl(str_url)) @@ -238,6 +241,7 @@ def test_successful_update(config_stub, basedir, download_stub, while host_blocker._in_progress: current_download = host_blocker._in_progress[0] with caplog.at_level(logging.ERROR): + current_download.successful = True current_download.finished.emit() host_blocker.read_hosts() assert_urls(host_blocker, whitelisted=[]) @@ -265,6 +269,8 @@ def test_failed_dl_update(config_stub, basedir, download_stub, # if current download is the file we want to fail, make it fail if current_download.name == dl_fail_blocklist.path(): current_download.successful = False + else: + current_download.successful = True with caplog.at_level(logging.ERROR): current_download.finished.emit() host_blocker.read_hosts() @@ -294,16 +300,18 @@ def test_invalid_utf8(config_stub, download_stub, tmpdir, data_tmpdir, host_blocker = adblock.HostBlocker() host_blocker.adblock_update() - finished_signal = host_blocker._in_progress[0].finished + current_download = host_blocker._in_progress[0] if location == 'content': with caplog.at_level(logging.ERROR): - finished_signal.emit() + current_download.successful = True + current_download.finished.emit() expected = (r"Failed to decode: " r"b'https://www.example.org/\xa0localhost") assert caplog.records[-2].message.startswith(expected) else: - finished_signal.emit() + current_download.successful = True + current_download.finished.emit() host_blocker.read_hosts() assert_urls(host_blocker, whitelisted=[])