Add a mock client to the cli test for Use #5203
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
|
Weirdly, the panic I ran into that caused me to create this PR does not seem to happen on Circle CI. |
|
After digging deeper, I found that the unit tests on circle do not run with parallelism. If you run them seriously, the tests don't break. After trying to find out what global state was getting shared, I found that the first unit tests called |
jsternberg
force-pushed
the
js-fix-use-test-panic
branch
from
1919011 to
2d3342f
Compare
|
I have now confirmed that all unit tests run for |
jsternberg
force-pushed
the
js-fix-use-test-panic
branch
2 times, most recently
from
46254db to
1b64b07
Compare
| @@ -66,9 +67,11 @@ func New(version string) *CommandLine { | |||
| } | |||
|
|
|||
| // Run executes the CLI | |||
| func (c *CommandLine) Run() { | |||
| func (c *CommandLine) Run() int { | |||
There was a problem hiding this comment.
Personally I'd rather see Run() return an error and the caller can write it out to os.Stderr. That would make it easier to evaluate which errors are returned from the tests.
One of the first unit tests in the cli tests called the Run method. Since the Run method called os.Exit, it reported the unit tests as succeeded. When parallel is set to 1, this skips _all_ unit tests after the first one. When parallel is set to a higher value, unit tests run by other processes still get run. This changes the Run method to return an error (if one occurred). This error can then be printed out and a bad exit status can be used to exit the program from the main program instead. That causes the unit tests to run correctly regardless of how many parallel processes are running. Also added an additional option to the CLI called `IgnoreSignals`. If this is set to true, then signals are not registered with the process. Setting signals doesn't really work in unit tests so it's good to ensure they don't get set in the first place. In addition to fixing the influx cli tests, this adds a mock client to the cli test for Use. PR #5183 added a validation for `use` to only be able to select public databases so `_internal` couldn't be chosen. To implement this, the `SHOW DATABASES` command was used by the internal client. Some of the unit tests in `cli_test.go` don't set the client to anything. `TestParseCommand_Use` previously didn't, but now it needs to have a client in the unit test with an empty test server.
jsternberg
force-pushed
the
js-fix-use-test-panic
branch
from
1b64b07 to
6b546cb
Compare
|
👍 |
jsternberg
added a commit
that referenced
this pull request
Jan 4, 2016
Add a mock client to the cli test for Use
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.
PR #5183 added a validation for
useto only be able to select publicdatabases so
_internalcouldn't be chosen. To implement this, theSHOW DATABASEScommand was used by the internal client.Some of the unit tests in
cli_test.godon't set the client toanything.
TestParseCommand_Useprevious didn't, but now it needs tohave a client in the unit test with an empty test server.