tests: remotes: use TmpDir-like fixtures #4140
Conversation
|
|
||
|
|
||
| @pytest.fixture | ||
| def workspace(tmp_dir, dvc, request): |
There was a problem hiding this comment.
will introduce a similar remote fixture as well instead of current explicit ones.
There was a problem hiding this comment.
Also, this really illustrates the need for #3920
| @@ -30,7 +33,7 @@ def get_url(): | |||
| def azure(): | |||
| if not Azure.should_test(): | |||
| pytest.skip("no azure running") | |||
| yield Azure() | |||
| yield Azure(Azure.get_url()) | |||
There was a problem hiding this comment.
This CLS(CLS.get_url()) mess will be removed once no tests use it. Likely in this PR.
| else: | ||
| path.write_text(contents, encoding="utf-8") | ||
|
|
||
| def gen(self, struct, text=""): |
There was a problem hiding this comment.
This will be unified with TmpDir in the future.
| @property | ||
| def _gc(self): | ||
| from google.cloud.storage import Client |
There was a problem hiding this comment.
Cloud fixtures try to implement methods by itself without relying on Tree too much, so that our tests are separated from our code. But sometimes it is just too hard to do that so in stuff like ssh, we use SSHConnection for now, but I might switch to plain paramiko after all, to make it more isolated.
| def is_file(self): | ||
| raise NotImplementedError | ||
|
|
||
| def is_dir(self): | ||
| raise NotImplementedError | ||
|
|
||
| def exists(self): | ||
| raise NotImplementedError | ||
|
|
||
| def mkdir(self, mode=0o777, parents=False, exist_ok=False): | ||
| raise NotImplementedError | ||
|
|
||
| def write_text(self, contents, encoding=None, errors=None): | ||
| raise NotImplementedError | ||
|
|
||
| def write_bytes(self, contents): | ||
| raise NotImplementedError | ||
|
|
||
| def read_text(self, encoding=None, errors=None): | ||
| raise NotImplementedError | ||
|
|
||
| def read_bytes(self): | ||
| raise NotImplementedError | ||
|
|
There was a problem hiding this comment.
All of these are your regular pathlib.Path's methods.
There was a problem hiding this comment.
For now we have a bare minimum, but it is easy to extend with what we need in the future.
| conn.close() | ||
|
|
||
| def is_file(self): | ||
| with self._ssh() as _ssh: |
There was a problem hiding this comment.
Not very nice, might just create a connection and then add a closing logic to close, as it is really intended to be, and then register a fixture finaliser or something.
|
For now running tests to see what i've missed in the ones that are not (easily) runnable locally. |
Codecov Report
@@ Coverage Diff @@
## master #4140 +/- ##
==========================================
- Coverage 92.71% 92.58% -0.13%
==========================================
Files 162 162
Lines 11293 11293
==========================================
- Hits 10470 10456 -14
- Misses 823 837 +14
Continue to review full report at Codecov.
|
efiop
force-pushed
the
test_external_outs
branch
6 times, most recently
from
77c6d59 to
69546ce
Compare
| self.dvc.update([import_remote_stage.path]) | ||
| self.assertEqual(self.dvc.status([import_remote_stage.path]), {}) | ||
|
|
||
| stages = self.dvc.reproduce(cmd_stage.addressing) |
There was a problem hiding this comment.
Got rid of the repro test in favor of add/import/update/run(no-exec) tests because running commands is not trivial in a unified way, as there is no mechanism of translating our remote:// notation to a user-consumable one. So let's not have it for now (we are not loosing much) until #3920 is looked into, as this problem again illustrates the need for proper workspaces.
efiop
changed the title
β I have followed the Contributing to DVC checklist.
π If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
β I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)
Thank you for the contribution - we'll try to review it as soon as possible. π