This repository was archived by the owner on Sep 3, 2025. It is now read-only.
Unit test GetRepositories() #11
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
derrickstolee
commented
Jan 31, 2023
vdye
reviewed
Jan 31, 2023
Collaborator
vdye
left a comment
vdye
left a comment
There was a problem hiding this comment.
These test updates and reorganization (and bugfix!) help us move towards a better overall framework for the bundle server, thanks for doing this! My comments are mainly around ways the changes can be simplified/take advantage of existing infrastructure - hopefully not too invasive.
derrickstolee
force-pushed
the
stolee/bundle-create-tests
branch
from
423a227 to
8813035
Compare
In an effort to make the codebase more unit-testable, we need to be able to mock certain lower layers of the system. We already have a FileSystem interface for interacting with the filesystem, but it is not yet robust enough for us to parse an existing file, as needed for GetRepositories(). Add a new ReadFileLines() method that takes a filename and returns a string slice (or error) and adapt GetRepositories() to use it. Note that this abstracts both os.OpenFile() and bufio.NewReader(). This should simplify the interface for most content interaction, but also will simplify the mocks we will eventually create for this method. Adjust all callers of GetRepositories() to provide a FileSystem, specifically the "real" one for now, until we can replace enough items with mockable interfaces that those methods can use an injected dependency at the command level. Helped-by: Victoria Dye <[email protected]> Signed-off-by: Derrick Stolee <[email protected]>
Continuing our efforts to make our code unit testable, add a user.User struct to the parameters for the methods in paths.go. This allows us to remove the rest of the dependencies on the real system in the GetRepositories() method. It requires some more work elsewhere in the codebase for all the places where the path methods are used. Signed-off-by: Derrick Stolee <[email protected]>
The shared_test namespace as added in 92237dd (systemd: implement 'Create()' function, 2023-01-10) and a4d43a6 (launchd: implement 'Create()' function, 2023-01-10) are helpful for unit testing logic. However, there are multiple barriers from using this mocking infrastructure in other tests, such as in internal/common: 1. The 'shared_test' namespace does not allow sharing anything beyond the internal/daemon/ directory. Moving it into just internal/ does not seem to make this visible. The "_test" portion of the package tells Go to treat it with special properties. 2. After moving the definitions into internal/commontest/, the definitions are not visible to anyone else because all the names start with a lowercase letter. Convert these to uppercase so they will be visible. 3. By extracting to an external package, we suddenly would need to add "commontest." in front of all of the imported terms. Use a trick in the import statement ('. "package"') to allow skipping this prefix. Signed-off-by: Derrick Stolee <[email protected]>
Create a unit test setup for the GetRepositories() method, to ensure that it works correctly with mocks. For this method, we only need to mock FileSystem.ReadFileLines() once, so we can have our tests store exactly one list of lines. Create a fake user.User struct to provide the HomeDir value. The only test implemented right now is the empty file. Signed-off-by: Derrick Stolee <[email protected]>
Add a few more cases to TestRepos_GetRepositories() including an error from the filesystem, a single repo case, and multiple repositories. Be sure to match the expected output of ReadFileLines() which is to trim the newlines. In the multiple repositories case, I added an empty line to check the case of skipping newlines. In an earlier version of this commit, I had thought there was a bug, but it was due to me including newlines in the mocked output. Signed-off-by: Derrick Stolee <[email protected]>
derrickstolee
force-pushed
the
stolee/bundle-create-tests
branch
from
8813035 to
de58f24
Compare
1 task
vdye
approved these changes
Feb 1, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I set out to learn more about Go and the unit testing framework that @vdye set up in github/git-fundamentals#9, with a focus on picking a very simple method from our core package:
GetRepositories().Along the way, @vdye helped me find a way to move a significant portion of
shared_test.gointo another place that can be reused in multiple places. Specifically, we will need the mocked structures. I useMockFileSystemin this PR, and had to extend it for the filesystem methods used inGetRepositories().Important things to note:
_test.go.. "<package>"in theimportstatement. This avoids an even worse transformation in the existing tests, which are already converting many lowercase types and methods into uppercase.Hopefully this large change to take the core of our unit-testing strategy and make it accessible to other packages will be relatively stable in future iterations. Then, we can hopefully result in more involved unit tests in the future that test things like the bundling strategy.