-
Notifications
You must be signed in to change notification settings - Fork 14
extend download tests #72
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
Conversation
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.
Do you think it would be worthy create a suite for download_test.go
with a setup and a teardown for the Helper
stuff so the directory structure would be reused across the tests or it would generate conflicts among them?
Anyway apart of the nitpicky comments LGTM! thanks!
downloader/download_test.go
Outdated
// 1) try to create *siva.NewLibrary using fs with broken OpenFile method | ||
// <expected> error that contains broken fs mocked error's text | ||
func TestLibraryCreationFailed(t *testing.T) { | ||
h, closer, err := testhelper.NewHelper() |
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.
maybe close
instead of closer
would fit better for a function
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.
Done
downloader/download_test.go
Outdated
require.NoError(t, err) | ||
|
||
h.FS = testhelper.NewBrokenFS(h.FS, testhelper.BrokenFSOptions{FailedOpen: true}) | ||
_, err = h.NewLibrary() |
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.
The Helper
struct already holds an instantiated library, couldn't we use that library for each test rather than instantiate a new one?
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.
Sometimes we create libraries based on the mockup fs, I did the next changes:
- Refactored all test to run with one suite using one helper
- after each test
close
is invoked - If we do not mock up fs - we will use default library(instantiated in constructor)
testPrivateRepo = &test{ | ||
locID: borges.LocationID("2ea758d7c7cbc249acfd6fc4a67f926cae28c10e"), | ||
repoIDs: []borges.RepositoryID{ | ||
borges.RepositoryID("github.com/lwsanty/super-private-privacy-keep-out"), |
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.
cool name for a repository 🤣
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 tried my best :)
1) add some coverage cases from src-d#62 (comment) 2) add fs mockup cases 3) refactor 4) add small documentation closes src-d#63 Signed-off-by: lwsanty <lwsanty@gmail.com>
2297095
to
1dd0fff
Compare
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 think it's fine as it is so we don't need to add more dependencies but when we need more complex functionality for suite tests we used to work with https://github.com/stretchr/testify#suite-package
closes #63
Signed-off-by: lwsanty lwsanty@gmail.com