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

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

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

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

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

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"])
 
 

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

tests/remotes/local.py

@@ -1,3 +1,4 @@
+# pylint: disable=cyclic-import
 import pytest
 
 from tests.basic_env import TestDvc

tests/remotes/ssh.py

@@ -78,7 +78,7 @@ class SSHMocked(Base):
         }
 
 
-@pytest.fixture(scope="session", autouse=True)
+@pytest.fixture
 def ssh_server():
     import mockssh
 

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

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"