Conversation
|
This change is |
benbardin
requested review from
jeffswenson,
rafiss and
rickystewart
and removed request for
dt and
jeffswenson
| } | ||
|
|
||
| } | ||
| return nil |
There was a problem hiding this comment.
unreachable code.
There was a problem hiding this comment.
Nit: I would rather there be return fmt.Errorf("could not find cockroach binary in archive") or something similar in case due to some bug it turns out to no longer be true that this is unreachable. I also don't fully understand when missing the final return statement is a compile error or not, I'd rather that not change under our feet due to a Go upgrade or similar.
| return nil, fmt.Errorf("%s failed to parse version: %w", testserverMessagePrefix, err) | ||
| } | ||
|
|
||
| startCmd := "start-single-node" |
There was a problem hiding this comment.
moved into the new else below
| } | ||
|
|
||
| } | ||
| return nil |
There was a problem hiding this comment.
Nit: I would rather there be return fmt.Errorf("could not find cockroach binary in archive") or something similar in case due to some bug it turns out to no longer be true that this is unreachable. I also don't fully understand when missing the final return statement is a compile error or not, I'd rather that not change under our feet due to a Go upgrade or similar.
| // TODO(janexing): Make sure the log is written to logDir instead of shown in console. | ||
| // Should be done once issue #109 is solved: | ||
| // https://github.com/cockroachdb/cockroach-go/issues/109 | ||
| nodes[i].stdout = filepath.Join(logsBaseDir, "cockroach.stdout") |
There was a problem hiding this comment.
Nit: there's some mild duplication here (this whole nodes[i] thing seems to be copy-pasted?) You could refactor into a helper function but the duplication doesn't look so severe that it seems absolutely necessary.
There was a problem hiding this comment.
Demo mode and regular mode have different flags, so I think a helper here might be more risky than, well, helpful. I'll add a comment in the code.
|
I'll add your suggestion to the unreachable-code thing, that makes sense. |
The hard-coded defaults here around job schedules are more reasonable for some unit tests.
3 participants
The hard-coded defaults here around job schedules are more reasonable for some unit tests.