-
Notifications
You must be signed in to change notification settings - Fork 1.3k
tests: use dir helpers on updater #2952
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,40 +1,61 @@ | ||
| import json | ||
| import os | ||
| import mock | ||
| import pytest | ||
|
|
||
| from dvc import __version__ | ||
| from dvc.updater import Updater | ||
|
|
||
|
|
||
| class MockResponse(object): | ||
| def __init__(self, json_data, status_code): | ||
| self.json_data = json_data | ||
| self.status_code = status_code | ||
| @pytest.fixture | ||
| def updater(dvc): | ||
| return Updater(dvc.dvc_dir) | ||
|
|
||
| def json(self): | ||
| return self.json_data | ||
|
|
||
| @mock.patch("requests.get") | ||
| def test_fetch(mock_get, updater): | ||
| mock_get.return_value.status_code = 200 | ||
| mock_get.return_value.json.return_value = {"version": __version__} | ||
|
|
||
| def mocked_requests_get(*args, **kwargs): | ||
| class MockResponse: | ||
| def __init__(self, json_data, status_code): | ||
| self.json_data = json_data | ||
| self.status_code = status_code | ||
|
|
||
| def json(self): | ||
| return self.json_data | ||
|
|
||
| return MockResponse({"version": __version__}, 200) | ||
|
|
||
|
|
||
| def test_fetch(dvc_repo, mocker): | ||
| updater = Updater(dvc_repo.dvc_dir) | ||
| assert not os.path.exists(updater.updater_file) | ||
|
|
||
| mock_get = mocker.patch("requests.get", side_effect=mocked_requests_get) | ||
| updater.fetch(detach=False) | ||
| mock_get.assert_called_once_with(Updater.URL, timeout=Updater.TIMEOUT_GET) | ||
|
|
||
| mock_get.assert_called_once_with(Updater.URL, timeout=Updater.TIMEOUT_GET) | ||
| assert os.path.isfile(updater.updater_file) | ||
|
|
||
| with open(updater.updater_file, "r") as fobj: | ||
| info = json.load(fobj) | ||
|
|
||
| assert info["version"] == __version__ | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "latest, current, result", | ||
| [ | ||
| ("0.20.8", "0.21.0", False), | ||
| ("0.20.8", "0.20.8", False), | ||
| ("0.20.8", "0.19.0", True), | ||
| ], | ||
| ) | ||
| def test_is_outdated(latest, current, result, updater): | ||
| updater.latest = latest | ||
| updater.current = current | ||
|
|
||
| assert updater._is_outdated() == result | ||
|
|
||
|
|
||
| @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) | ||
|
|
||
| updater.check() | ||
| updater.check() | ||
| updater.check() | ||
|
Comment on lines
+57
to
+59
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we do this 3 times? It would be nice to have a comment explaining this.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. indeed, I imagine that is to "test" the lock mechanism or something. @efiop , could you give more details about this?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIRC, to check that we hit the logic when there is no updater file, when there is one and once more. It is pretty old code, so sorry for it being weird like that. 🙂
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, if all 3
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Suor Yes. |
||
|
|
||
| assert mock_check.call_count == 3 | ||
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.
move
.is_outdatedand.checkto the unit tests, since they are testing a single unit, in this case, a method.