Don't update dependencies more than once #132
Merged
Conversation
This file contains 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
As part of the work on #120, an error when using the update command, I wanted to add tests to demonstrate the bug and then show that the bug was fixed by my changes. This is problematic because there are no real unit tests for any commands. This commit is the first step in providing better unit testability of various parts of corral in general and commands specifically. The only test added by this commit is that when using a corral.json with empty deps, that no VCS operations will be executed by _Updater. There are a number of changes that are brought in my this initial refactor. As we move forward with making corral more testable, some of these changes will probably fall away. I did what I considered the minimal thing to get a harness for testing _Updater working. This harness should be enough to test using the various test-data bundles that we have as well as testing issue #120. The approach I have taken does leave a lot of things in an inconsistent state. For example, this commit introduces a new way of organizing unit tests that keeps said tests near the code they are testing and allows for testing private implementations like the _Update that is part of the update cmd. These inconsistencies are temporary and as we move forward with adding more tests, will start to fall away.
SeanTAllen
added
changelog - fixed
|
This PR needs to be squashed and the top level GH comment should be used as the body of the squash and the PR title should be the commit title. |
Theodus
approved these changes
Jun 7, 2020
| let recorder = _OpsRecorder(h, 4, 4, 4) | ||
| let vcs_builder: VCSBuilder = _TestCmdUpdateVCSBuilder(recorder) | ||
|
|
||
| // when |
There was a problem hiding this comment.
Yes...
There's also a "given" above before the setup so Given this setup, when Blah...
from a given, when, then style.
If you think it is confusing, I can remove.
There was a problem hiding this comment.
I think it can stay, since I don't think it would hinder anyone's understanding of the test
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Prior to this commit, when update would run, if there were transitive dependencies, it might try to fetch/update them more than once potentially leading to errors.
This commit adds a regression unit test that shows the problem and a simple fix to the _Updater to handle. I use "simple" to mean "small" or "baby-steps". The internal design of _Updater needs to be examined in more depth in order to make this all better.
I'm not tackling that now. Before getting into design, we need to add more tests in general to see what affordances for insight and tracing that corral needs to provide then we can build the design around that.
I wanted to add tests to demonstrate the bug and then show that the
bug was fixed by my changes. This was problematic because there are no real unit tests for any
commands.
This PR includes a first step in providing better unit testability
of various parts of corral in general and commands specifically.
There are a number of testing-related changes that are brought this PR.
As we move forward with making corral more testable, some of these changes
will probably fall away. I did what I considered the minimal thing to get
a unit-test harness for testing _Updater working.
The approach I have taken does leave a lot of things in an inconsistent state.
For example, this PR introduces a new way of organizing unit tests that keeps
said tests near the code they are testing and allows for testing private implementations
like the _Update that is part of the update cmd.
These inconsistencies are temporary and as we move forward with adding more tests, will
start to fall away.
Closes #120