Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpk start: add tests #270

Merged
merged 3 commits into from
Dec 28, 2020
Merged

rpk start: add tests #270

merged 3 commits into from
Dec 28, 2020

Conversation

0x5d
Copy link
Contributor

@0x5d 0x5d commented Dec 10, 2020

Make rpk start testable by injecting the redpanda.Launcher instance instead of creating it when the command runs, allowing for mocking it and recreating various scenarios.

Fix #213

@0x5d 0x5d requested review from BenPope and 0xdiba December 10, 2020 22:52
@0x5d 0x5d self-assigned this Dec 10, 2020
@0x5d 0x5d marked this pull request as draft December 10, 2020 22:52
@0x5d 0x5d force-pushed the rpk-start-tests branch 7 times, most recently from 2d36602 to cd0a838 Compare December 11, 2020 21:25
@0x5d 0x5d marked this pull request as ready for review December 11, 2020 21:27
@emaxerrno
Copy link
Contributor

@0x5d this is more of a discussion, but what about other commands.

Can you describe what injection points are left.

I imagine

  1. Environment
  2. Filesystem
  3. Commands
  4. Plugins
    4.1 Actual commands/ cat | exec | tee / etc
  5. Custom per OS extensions

would all basically be under different interfaces

@0x5d
Copy link
Contributor Author

0x5d commented Dec 21, 2020

@senior7515 so for rpk start (reposting here from Slack):

Environment: covered.
FS & actual commands: used by the check & tune steps in ‘start’. They're not covered by these tests yet.
Per-OS extensions: rpk start is itself OS-dependent (only built for Linux).

@0x5d
Copy link
Contributor Author

0x5d commented Dec 22, 2020

@senior7515 can I merge this?

@0x5d 0x5d added the area/rpk label Dec 22, 2020
Copy link
Member

@BenPope BenPope left a comment

Choose a reason for hiding this comment

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

The changes seem fine. I didn't review the tests.

@BenPope
Copy link
Member

BenPope commented Dec 22, 2020

You could have a test for --overprovisioned flag overrides config overprovisioned: false ;) #304

Also, there are other flags, like node_id, smp, memory, etc.

src/go/rpk/pkg/cli/cmd/start_test.go Outdated Show resolved Hide resolved
src/go/rpk/pkg/cli/cmd/start_test.go Outdated Show resolved Hide resolved
src/go/rpk/pkg/cli/cmd/start_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@0xdiba 0xdiba left a comment

Choose a reason for hiding this comment

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

LGTM pending Ben's comments.

my 2 cents, probably out of scope for this PR:
I can't help but wonder how we could make configuration testing easier. Having tests for every flag to verify that Get value from env if not provided through cli works should be tested centrally in some way and as a whole for the configuration 🤔

David Castillo added 2 commits December 23, 2020 13:18
Change redpanda/launcher to receive the install dir & redpanda
args in Start, as opposed to when it's initialized, since it
doesn't require them then. This will allow to change cmd.Start
to receive a Launcher instance, making it possible to unit test it.
@0x5d
Copy link
Contributor Author

0x5d commented Dec 23, 2020

LGTM pending Ben's comments.

my 2 cents, probably out of scope for this PR:
I can't help but wonder how we could make configuration testing easier. Having tests for every flag to verify that Get value from env if not provided through cli works should be tested centrally in some way and as a whole for the configuration thinking

Yeah. Ben and I were discussing that in a previous PR. The config.Manager api & initialization needs to change to account for these different config sources.
But I still think that there should be tests at the outermost layers (i.e. commands) checking that the command actually does the right thing regarding config source hierarchies and parsing.

@0x5d
Copy link
Contributor Author

0x5d commented Dec 23, 2020

You could have a test for --overprovisioned flag overrides config overprovisioned: false ;) #304

I will fix this in another PR :D

Also, there are other flags, like node_id, smp, memory, etc.

Yeah, but these don't need to be parsed and checking them is harder, since they're not persisted but passed to redpanda directly. I think to effectively test them it has to be with integration tests.

@0x5d 0x5d merged commit 9dabd76 into redpanda-data:dev Dec 28, 2020
@0x5d 0x5d deleted the rpk-start-tests branch December 28, 2020 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make rpk start testable
4 participants