From 047352027f51b922f3124b52f7ff9f77e8470b14 Mon Sep 17 00:00:00 2001 From: Saugat Pachhai Date: Fri, 26 Jun 2020 14:22:38 +0000 Subject: [PATCH] pylint: enable attribute-outside-init check (#4119) * pylint: enable attribute-outside-init check * fix tests * Fix mock for time This was an interesting one. I was mocking `time` on function definition and then, using those later on test run. When all the tests were run, there would be some delay that'd make tests fail. Hmmm ... --- .pylintrc | 27 +++++----- dvc/updater.py | 17 +++--- tests/basic_env.py | 6 ++- tests/dir_helpers.py | 2 +- tests/func/test_add.py | 19 ++++--- tests/func/test_repro.py | 16 +++--- tests/remotes/local.py | 1 + tests/remotes/ssh.py | 2 +- tests/unit/test_updater.py | 80 ++++++++++++++++++++++------ tests/unit/utils/test_collections.py | 2 +- 10 files changed, 110 insertions(+), 62 deletions(-) diff --git a/.pylintrc b/.pylintrc index f8f0ed3d8..08f96e765 100644 --- a/.pylintrc +++ b/.pylintrc @@ -149,8 +149,11 @@ disable=format, exception-escape, comprehension-escape, # custom disables start here + protected-access, # needs-refactor + no-self-use, invalid-name, blacklisted-name, + # already a check enabled for `wildcard-import` unused-wildcard-import, misplaced-comparison-constant, # probably soon: https://github.com/iterative/dvc/issues/1843 @@ -162,21 +165,21 @@ disable=format, missing-module-docstring, missing-class-docstring, duplicate-code, + # too noisy, and detects issues even when import is inside a function + # see: https://github.com/PyCQA/pylint/issues/850 cyclic-import, - wrong-import-order, # using isort - wrong-import-position, # using isort - ungrouped-imports, # using isort - multiple-imports, # isort - import-outside-toplevel, # we use it to speedup/optimize + # isort should cover these: + wrong-import-order, + wrong-import-position, + ungrouped-imports, + multiple-imports, + # we use it to speedup/optimize + import-outside-toplevel, fixme, - # Enabling these soon: - attribute-defined-outside-init, # needs-refactor - # Enabling someday: - no-member, # needs-refactor - unused-argument, # need to cleanup - protected-access, # needs-refactor + # Enabling soon: + no-member, + unused-argument, arguments-differ, - no-self-use, # Enable the message, report, category or checker with the given id(s). You can diff --git a/dvc/updater.py b/dvc/updater.py index ca8e7a17c..4230a2951 100644 --- a/dvc/updater.py +++ b/dvc/updater.py @@ -61,15 +61,15 @@ class Updater: # pragma: no cover try: info = json.load(fobj) - self.latest = info["version"] + latest = info["version"] except Exception as exc: # pylint: disable=broad-except msg = "'{}' is not a valid json: {}" logger.debug(msg.format(self.updater_file, exc)) self.fetch() return - if self._is_outdated(): - self._notify() + if version.parse(self.current) < version.parse(latest): + self._notify(latest) def fetch(self, detach=True): from dvc.daemon import daemon @@ -85,8 +85,8 @@ class Updater: # pragma: no cover import requests try: - r = requests.get(self.URL, timeout=self.TIMEOUT_GET) - info = r.json() + resp = requests.get(self.URL, timeout=self.TIMEOUT_GET) + info = resp.json() except requests.exceptions.RequestException as exc: msg = "Failed to retrieve latest version: {}" logger.debug(msg.format(exc)) @@ -95,10 +95,7 @@ class Updater: # pragma: no cover with open(self.updater_file, "w+") as fobj: json.dump(info, fobj) - def _is_outdated(self): - return version.parse(self.current) < version.parse(self.latest) - - def _notify(self): + def _notify(self, latest): if not sys.stdout.isatty(): return @@ -113,7 +110,7 @@ class Updater: # pragma: no cover yellow=colorama.Fore.YELLOW, blue=colorama.Fore.BLUE, current=self.current, - latest=self.latest, + latest=latest, ) logger.info(boxify(message, border_color="yellow")) diff --git a/tests/basic_env.py b/tests/basic_env.py index 1f87a9501..78ccc0230 100644 --- a/tests/basic_env.py +++ b/tests/basic_env.py @@ -46,13 +46,14 @@ class TestDirFixture: def __init__(self): root_dir = self.mkdtemp() self._root_dir = os.path.realpath(root_dir) + self._saved_dir = None @property def root_dir(self): return self._root_dir def _pushd(self, d): - if not hasattr(self, "_saved_dir"): + if not self._saved_dir: self._saved_dir = os.path.realpath(os.curdir) os.chdir(d) @@ -96,6 +97,7 @@ class TestDirFixture: try: remove(self._root_dir) except OSError as exc: + # pylint: disable=no-member # We ignore this under Windows with a warning because it happened # to be really hard to trace all not properly closed files. # @@ -189,6 +191,7 @@ class TestDvc(TestDvcFixture, TestCase): def __init__(self, methodName): TestDvcFixture.__init__(self) TestCase.__init__(self, methodName) + self._caplog = None @pytest.fixture(autouse=True) def inject_fixtures(self, caplog): @@ -199,6 +202,7 @@ class TestDvcGit(TestDvcGitFixture, TestCase): def __init__(self, methodName): TestDvcGitFixture.__init__(self) TestCase.__init__(self, methodName) + self._caplog = None @pytest.fixture(autouse=True) def inject_fixtures(self, caplog): diff --git a/tests/dir_helpers.py b/tests/dir_helpers.py index e28bc0c0e..4395a2a02 100644 --- a/tests/dir_helpers.py +++ b/tests/dir_helpers.py @@ -43,7 +43,7 @@ from global repo template to creating everything inplace, which: - does not create unnecessary files """ -# pylint: disable=redefined-outer-name +# pylint: disable=redefined-outer-name, attribute-defined-outside-init import os import pathlib diff --git a/tests/func/test_add.py b/tests/func/test_add.py index c016092dd..c681b6129 100644 --- a/tests/func/test_add.py +++ b/tests/func/test_add.py @@ -359,27 +359,26 @@ class SymlinkAddTestBase(TestDvc): def _prepare_external_data(self): data_dir = self._get_data_dir() - self.data_file_name = "data_file" - external_data_path = os.path.join(data_dir, self.data_file_name) + data_file_name = "data_file" + external_data_path = os.path.join(data_dir, data_file_name) with open(external_data_path, "w+") as f: f.write("data") - self.link_name = "data_link" - System.symlink(data_dir, self.link_name) + link_name = "data_link" + System.symlink(data_dir, link_name) + return data_file_name, link_name def _test(self): - self._prepare_external_data() + data_file_name, link_name = self._prepare_external_data() - ret = main(["add", os.path.join(self.link_name, self.data_file_name)]) + ret = main(["add", os.path.join(link_name, data_file_name)]) self.assertEqual(0, ret) - stage_file = self.data_file_name + DVC_FILE_SUFFIX + stage_file = data_file_name + DVC_FILE_SUFFIX self.assertTrue(os.path.exists(stage_file)) d = load_yaml(stage_file) - relative_data_path = posixpath.join( - self.link_name, self.data_file_name - ) + relative_data_path = posixpath.join(link_name, data_file_name) self.assertEqual(relative_data_path, d["outs"][0]["path"]) diff --git a/tests/func/test_repro.py b/tests/func/test_repro.py index 160da4a58..64e5a66f7 100644 --- a/tests/func/test_repro.py +++ b/tests/func/test_repro.py @@ -231,27 +231,25 @@ class TestReproWorkingDirectoryAsOutput(TestDvc): class TestReproDepUnderDir(SingleStageRun, TestDvc): def test(self): stages = self.dvc.add(self.DATA_DIR) - self.assertEqual(len(stages), 1) - self.dir_stage = stages[0] - self.assertTrue(self.dir_stage is not None) + self.assertTrue(stages and stages[0] is not None) - self.file1 = "file1" + file1 = "file1" stage = self._run( - fname=self.file1 + ".dvc", - outs=[self.file1], + fname=file1 + ".dvc", + outs=[file1], deps=[self.DATA, self.CODE], - cmd=f"python {self.CODE} {self.DATA} {self.file1}", + cmd=f"python {self.CODE} {self.DATA} {file1}", name="copy-data-file1", ) - self.assertTrue(filecmp.cmp(self.file1, self.DATA, shallow=False)) + self.assertTrue(filecmp.cmp(file1, self.DATA, shallow=False)) os.unlink(self.DATA) shutil.copyfile(self.FOO, self.DATA) stages = self.dvc.reproduce(self._get_stage_target(stage)) self.assertEqual(len(stages), 2) - self.assertTrue(filecmp.cmp(self.file1, self.FOO, shallow=False)) + self.assertTrue(filecmp.cmp(file1, self.FOO, shallow=False)) class TestReproDepDirWithOutputsUnderIt(SingleStageRun, TestDvc): diff --git a/tests/remotes/local.py b/tests/remotes/local.py index b2df8f089..a99cc26c2 100644 --- a/tests/remotes/local.py +++ b/tests/remotes/local.py @@ -1,3 +1,4 @@ +# pylint: disable=cyclic-import import pytest from tests.basic_env import TestDvc diff --git a/tests/remotes/ssh.py b/tests/remotes/ssh.py index fc5da2966..c0514b363 100644 --- a/tests/remotes/ssh.py +++ b/tests/remotes/ssh.py @@ -78,7 +78,7 @@ class SSHMocked(Base): } -@pytest.fixture(scope="session", autouse=True) +@pytest.fixture def ssh_server(): import mockssh diff --git a/tests/unit/test_updater.py b/tests/unit/test_updater.py index 2b9926037..76257af66 100644 --- a/tests/unit/test_updater.py +++ b/tests/unit/test_updater.py @@ -1,5 +1,7 @@ import json +import logging import os +import time import mock import pytest @@ -8,9 +10,20 @@ from dvc import __version__ from dvc.updater import Updater +@pytest.fixture(autouse=True) +def mock_env(monkeypatch): + monkeypatch.delenv("CI", None) + monkeypatch.setenv("DVC_TEST", "False") + + @pytest.fixture -def updater(dvc): - return Updater(dvc.dvc_dir) +def updater(tmp_path): + return Updater(tmp_path) + + +@pytest.fixture +def mock_tty(mocker): + return mocker.patch("sys.stdout.isatty", return_value=True) @mock.patch("requests.get") @@ -32,29 +45,62 @@ def test_fetch(mock_get, updater): @pytest.mark.parametrize( - "latest, current, result", + "current,latest,notify", [ - ("0.20.8", "0.21.0", False), - ("0.20.8", "0.20.8", False), - ("0.20.8", "0.19.0", True), + ("0.0.2", "0.0.2", False), + ("0.0.2", "0.0.3", True), + ("0.0.2", "0.0.1", False), ], + ids=["uptodate", "behind", "ahead"], ) -def test_is_outdated(latest, current, result, updater): - updater.latest = latest +def test_check_updates(mock_tty, updater, caplog, current, latest, notify): updater.current = current + with open(updater.updater_file, "w+") as f: + json.dump({"version": latest}, f) - assert updater._is_outdated() == result + caplog.clear() + with caplog.at_level(logging.INFO, logger="dvc.updater"): + updater.check() + + if notify: + assert f"Update available {current} -> {latest}" in caplog.text + else: + assert not caplog.text + + +def test_check_refetches_each_day(mock_tty, updater, caplog, mocker): + updater.current = "0.0.8" + with open(updater.updater_file, "w+") as f: + json.dump({"version": "0.0.9"}, f) + fetch = mocker.patch.object(updater, "fetch") + + time_value = time.time() + 24 * 60 * 60 + 10 + mock_time = mocker.patch("time.time", return_value=time_value) + + caplog.clear() + with caplog.at_level(logging.INFO, logger="dvc.updater"): + updater.check() + assert not caplog.text + fetch.assert_called_once() + mock_time.assert_called() + + +def test_check_fetches_on_invalid_data_format( + mock_tty, updater, caplog, mocker +): + updater.current = "0.0.5" + with open(updater.updater_file, "w+") as f: + f.write('"{"version: "0.0.6"') + fetch = mocker.patch.object(updater, "fetch") + caplog.clear() + with caplog.at_level(logging.INFO, logger="dvc.updater"): + updater.check() + assert not caplog.text + fetch.assert_called_once() -@pytest.mark.skipif( - os.getenv("TRAVIS_EVENT_TYPE") != "cron", - reason="Only run on travis CRON to avoid generating too much logs", -) @mock.patch("dvc.updater.Updater._check") -def test_check(mock_check, updater, monkeypatch): - monkeypatch.delenv("CI", None) - monkeypatch.setenv("DVC_TEST", False) - +def test_check(mock_check, updater): updater.check() updater.check() updater.check() diff --git a/tests/unit/utils/test_collections.py b/tests/unit/utils/test_collections.py index 3a296bf25..3efaa6e1a 100644 --- a/tests/unit/utils/test_collections.py +++ b/tests/unit/utils/test_collections.py @@ -12,7 +12,7 @@ class MyInt(int): def test_apply_diff_is_inplace(): dest = MyDict() - dest.attr = 42 + dest.attr = 42 # pylint: disable=attribute-defined-outside-init apply_diff({}, dest) assert type(dest) is MyDict, "Preserves class"