-
Notifications
You must be signed in to change notification settings - Fork 71
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
chore(fs): add tests to cover recent PRs #328
Conversation
e05901d
to
1a3c1a1
Compare
1a3c1a1
to
99985b5
Compare
hint="Confirm the directory exists and you can access it.", | ||
), | ||
} | ||
cache = {"dirs": defaultdict(list), "ids": {}} |
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.
C: had to cleanup this a bit (root id is not used anymore, also we now cache ids
not a single id
even for the base
99985b5
to
b687d83
Compare
@iterative/dvc a gentle reminder to review this please. |
|
||
|
||
@pytest.fixture(scope="module") | ||
def fs_factory(base_remote_dir, service_auth): |
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.
I am still not sure if we really need fs_factory()
fixture. Readability and straightforward test is more important than the duplication.
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.
hmm, what is the alternative? I can still explore that in the followup ...
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.
You can initialize the base_remote_dir
automatically in another fixture (with module
scope and use it in fs
. This will only be called once.
@pytest.fixture(scope="module")
def base_dir(base_remote_dir):
fs = GDriveFileSystem(base_remote_dir, service_auth)
fs._gdrive_create_dir(...)
yield remote_dir
# cleanup
@pytest.fixture
def fs(base_dir):
...
Then, you can just do fs = GDriveFileSystem(base_remote_dir, service_auth)
wherever you are doing fs_factory()
, right? Or, did I miss something here?
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.
okay, I see. There are a few other things (e.g. I'm disabling fs caching in the fixture), probably it all can be moved to the base_dir or something, but I'm not sure it make a big difference
it's easier tbh for me to have all this logic in one place - easier to do changes.
Tests for the recent PR regressions:
#322
#321
Also seems that #229 is fixed now. This PR also covers a basic test for it. @simone-viozzi please confirm if / when you have time.
Closes #229