Skip to content
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

Merged
merged 1 commit into from
Jan 3, 2024
Merged

chore(fs): add tests to cover recent PRs #328

merged 1 commit into from
Jan 3, 2024

Conversation

shcheklein
Copy link
Member

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

hint="Confirm the directory exists and you can access it.",
),
}
cache = {"dirs": defaultdict(list), "ids": {}}
Copy link
Member Author

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

pydrive2/test/test_fs.py Outdated Show resolved Hide resolved
@shcheklein
Copy link
Member Author

@iterative/dvc a gentle reminder to review this please.



@pytest.fixture(scope="module")
def fs_factory(base_remote_dir, service_auth):
Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

@skshetry skshetry Jan 3, 2024

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?

Copy link
Member Author

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.

@shcheklein shcheklein merged commit 7074aba into main Jan 3, 2024
16 checks passed
@shcheklein shcheklein deleted the add-fs-tests branch January 3, 2024 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem with the cache in GDriveFileSystem.find
2 participants