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

integration_test: add more test cases #297

Merged
merged 2 commits into from Jul 23, 2019

Conversation

@ahmetb
Copy link
Member

commented Jul 23, 2019

Added more tests or expanded existing tests to:

  • cover args/stdin handling edge cases for install cmd
  • cover status code of re-runs of install, update, and remove
  • cover status when index not initialized in remove, upgrade and install
  • several other minor adjustments

Fixes #294
Fixes #295

integration_test: add more test cases
Added more tests or expanded existing tests to:

- cover args/stdin handling edge cases for `install` cmd
- cover status code of re-runs of `install`, `update`, and `remove`.
- cover status when index not initialized in `remove`, `upgrade`, `install`
- several other minor adjustments

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>

@ahmetb ahmetb force-pushed the ahmetb:cmd-tests branch from d279a6e to 0303ff2 Jul 23, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Jul 23, 2019

Codecov Report

Merging #297 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #297   +/-   ##
=======================================
  Coverage   56.22%   56.22%           
=======================================
  Files          19       19           
  Lines         891      891           
=======================================
  Hits          501      501           
  Misses        337      337           
  Partials       53       53

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86a9199...528968f. Read the comment docs.

@corneliusweig
Copy link
Contributor

left a comment

Nice work, looks excellent.

Of the points listed in the referenced issues, I cannot find anything about

list: test if plugin names can be saved to a file properly when STDOUT is
not a tty (i.e. 'backup' use case)

Was that intentional? Myself, I cannot think of a way how to get proper output for ttys, so it looks not testable to me.


test, cleanup := NewTest(t)
defer cleanup()

test.WithIndex().Krew("install", validPlugin).RunOrFailOutput()
test.WithIndex().Krew("install", validPlugin, validPlugin2).RunOrFailOutput()

This comment was marked as resolved.

Copy link
@corneliusweig

corneliusweig Jul 23, 2019

Contributor

OUTDATED

Suggested change
test.WithIndex().Krew("install", validPlugin, validPlugin2).RunOrFailOutput()
test.Krew("install", validPlugin, validPlugin2).RunOrFailOutput()

If you want to keep the symmetry, this is fine, but it's not necessary.

@ahmetb

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

Was that intentional? Myself, I cannot think of a way how to get proper output for ttys, so it looks not testable to me.

I think krew list > file.txt is supposed to be printing only names (not a table). Let's check.

@ahmetb

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

yep:

k krew list | cat
get-all
konfig
krew
mtail
view-secret
who-can

I'll add a test for this as well.

@ahmetb

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

krew list is actually tested fine, I'm now updating its tests to be more precise.

Update TestKrewList to parse structured output
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
@ahmetb

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

Fixed now.

@corneliusweig

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

You are really lifting krew's code quality to the next level. 👏 for your efforts!
/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link

commented Jul 23, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, corneliusweig

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [ahmetb,corneliusweig]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit a89c34b into kubernetes-sigs:master Jul 23, 2019

3 checks passed

cla/linuxfoundation ahmetb authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.