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

tests: abstract remote setup to fixture #3748

Merged
merged 1 commit into from
May 11, 2020

Conversation

pared
Copy link
Contributor

@pared pared commented May 6, 2020

  • ❗ 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. If the CLI API is changed, I have updated tab completion scripts.

  • ❌ 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. πŸ™
Fixes #2888, related to #3416

The problem:
In #3416 I could reproduce user problems in script, but it seemed that I am unable to write a test for it.
The problem turned out to be related to the fact that erepo_dir has been setting its default remote to be its cache. Due to that, I have been indirectly using cache "read only mode" optimization, while using remote. Because it is the second time i stumble upon this issue (#2888) I decided to fix it.
This issue introduces "setup_remote" fixture, which is intended to be default way to set up the remote for particular Repo instance.

@pared pared requested review from efiop, skshetry and pmrowla May 6, 2020 11:52
tests/dir_helpers.py Outdated Show resolved Hide resolved
@pared pared requested a review from skshetry May 6, 2020 15:00
@pared pared changed the title tests: erepo_dir: extract remote setup to separate method tests: remote setup to separate method in TmpDir May 6, 2020
@pared pared force-pushed the fix_erepo_remote branch from 9178b43 to 70b0a7b Compare May 6, 2020 15:42
@pared pared mentioned this pull request May 7, 2020
3 tasks
@pared pared force-pushed the fix_erepo_remote branch 2 times, most recently from ebe2e8f to 03b5e99 Compare May 8, 2020 10:44
@pared pared changed the title tests: remote setup to separate method in TmpDir tests: abstract remote setup to fixture May 8, 2020
@pared pared force-pushed the fix_erepo_remote branch from 03b5e99 to a0d6ab0 Compare May 8, 2020 11:49
Copy link
Member

@skshetry skshetry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a cleanup. Looks good otherwise @pared.

tests/dir_helpers.py Outdated Show resolved Hide resolved
tests/dir_helpers.py Outdated Show resolved Hide resolved
Copy link
Contributor

@pmrowla pmrowla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other than the minor things already noted, looks good

@pared pared force-pushed the fix_erepo_remote branch from e28809f to b86700c Compare May 11, 2020 18:36
@pared pared requested a review from skshetry May 11, 2020 18:37
@efiop efiop merged commit 451c7aa into iterative:master May 11, 2020
@pared pared deleted the fix_erepo_remote branch January 4, 2022 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tests: erepo upstream should not be its cache
4 participants