Skip to content

Conversation

@efiop
Copy link
Contributor

@efiop efiop commented May 7, 2023

Followup for #9415

@efiop efiop marked this pull request as ready for review May 7, 2023 21:16
@codecov
Copy link

codecov bot commented May 7, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.25 ⚠️

Comparison is base (967dd5e) 91.59% compared to head (6c98eaf) 91.34%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9417      +/-   ##
==========================================
- Coverage   91.59%   91.34%   -0.25%     
==========================================
  Files         487      487              
  Lines       37779    37780       +1     
  Branches     5436     5436              
==========================================
- Hits        34602    34511      -91     
- Misses       2620     2693      +73     
- Partials      557      576      +19     
Impacted Files Coverage Δ
tests/func/test_import.py 99.46% <100.00%> (+<0.01%) ⬆️

... and 23 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

Thanks, Ruslan. Assuming the "by design" behaviour of *FS is tested in their corresponding tests, it's good and should be enough. (I would personally still sleep better with some integration tests that cover such things).

@efiop
Copy link
Contributor Author

efiop commented May 7, 2023

@shcheklein I've thought about a more comprehensive test, but couldn't come up with something comprehensive but also quick, not fragile and reasonably simple. Might come back in the future. Same with tweaking the current benchmark. I will keep it in mind.

@efiop efiop merged commit 6b6084a into main May 7, 2023
@efiop efiop deleted the efiop-patch-1 branch May 7, 2023 21:34
@efiop efiop added testing Related to the tests and the testing infrastructure skip-changelog Skips changelog labels May 7, 2023
@efiop efiop self-assigned this May 7, 2023
@skshetry
Copy link
Collaborator

skshetry commented May 8, 2023

I think this is dvcfs/datafs design issue, as there's always a default cache. If it did support passing an odb, we would not have to pass the config. Also, passing concrete types are always better than passing config dicts.

https://github.com/iterative/dvc/blob/6b6084a84829ba90b740a1cb18ed897d96fcc3af/dvc/fs/dvc.py#L260-L261

We would not have to do things like this for plots if it had a notion of default cache.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changelog Skips changelog testing Related to the tests and the testing infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants