registry/cshoop/storagedriver Added StorageDriver method WalkFiles, optimize blobstore enumeration with S3 #16
Conversation
CollinShoop
changed the title
|
|
||
| // WalkFiles traverses a filesystem defined within driver, starting | ||
| // from the given path, calling f on each file | ||
| func (d *driver) WalkFiles(ctx context.Context, from string, f storagedriver.WalkFn) error { |
There was a problem hiding this comment.
Should we update the walk_test for this slightly different logic?
There was a problem hiding this comment.
There aren't even tests for Walk really. I don't quite want to get into the work of adding tests for them but could if it's worth the effort
There was a problem hiding this comment.
Yeah good point. But still worth the consideration, especially with regards to preventing regressions in functionality. Lack of proper testing for current functionality is never a reason for not adding tests 😉
There was a problem hiding this comment.
Turns out my fallback method was actually incorrect. Added a bunch of unit tests to check the behavior more explicitly.
There was a problem hiding this comment.
I'd like to add S3 tests as well but that's way more involved, I don't want to spent too too long on this... 😭
…rking & added a Files Removed test for WalkFilesFallback.
| @@ -45,3 +72,217 @@ func TestWalkFileRemoved(t *testing.T) { | |||
| t.Fatalf(err.Error()) | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
In retrospect, I probably could have made this table drive, but oh well 🤷🏻♂️ lmk if this is worth changing ?
There was a problem hiding this comment.
Great job on the decision to add test coverage!
I think your recent comment was correct in thinking that maybe these could’ve been made as table tests. It’s a combination of personal taste and practicality. But, I personally prefer them for their readability, maintainability and adaptability.
I think reading Dave Cheney’s take on table test preference is a good read and may provide more insight than I can provide in these GitHub comments. It might be worth the consideration to better organize the structure and reduce some of the repetition.
|
Moved to #17 |
Thanks Jeremy, will address testing changes in the new PR |
Moved to #17