Skip to content

Conversation

lwsanty
Copy link
Contributor

@lwsanty lwsanty commented Sep 13, 2019

  1. add some coverage cases from downloader: refactor downloadRepository function #62 (comment)
  2. add fs mockup cases
  3. refactor
  4. add small documentation

closes #63

Signed-off-by: lwsanty lwsanty@gmail.com

@mcarmonaa mcarmonaa requested a review from a team September 13, 2019 10:43
Copy link
Contributor

@mcarmonaa mcarmonaa left a 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!

// 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()
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

require.NoError(t, err)

h.FS = testhelper.NewBrokenFS(h.FS, testhelper.BrokenFSOptions{FailedOpen: true})
_, err = h.NewLibrary()
Copy link
Contributor

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?

Copy link
Contributor Author

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"),
Copy link
Contributor

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 🤣

Copy link
Contributor Author

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>
Copy link
Contributor

@mcarmonaa mcarmonaa left a 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

@mcarmonaa mcarmonaa requested a review from jfontan September 16, 2019 08:38
@mcarmonaa mcarmonaa merged commit 1b68b71 into src-d:master Sep 17, 2019
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.

Improve tests
3 participants