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

More integration tests #208

Merged

Conversation

Projects
None yet
4 participants
@corneliusweig
Copy link
Contributor

commented Jun 6, 2019

These are based on #203 and should be merged after that one. Reviewing before #203 is merged makes also no sense :-/

The tests currently cover the happy path of all subcommands excluding krew upgrade. Upgrade is a special case because it needs two versions of a valid plugin. That will be difficult to set up and may be better in a separate PR.

@k8s-ci-robot k8s-ci-robot requested review from ahmetb and juanvallejo Jun 6, 2019

@corneliusweig corneliusweig force-pushed the corneliusweig:more-integration-tests branch from 11d8700 to 31d3f2f Jun 6, 2019

@corneliusweig corneliusweig marked this pull request as ready for review Jun 6, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Jun 6, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #208   +/-   ##
=======================================
  Coverage   53.77%   53.77%           
=======================================
  Files          13       13           
  Lines         716      716           
=======================================
  Hits          385      385           
  Misses        279      279           
  Partials       52       52

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 04288cb...287a50b. Read the comment docs.

Show resolved Hide resolved docs/CONTRIBUTOR_GUIDE.md Outdated
Show resolved Hide resolved hack/run-integration-tests.sh Outdated

corneliusweig added some commits Jun 6, 2019

Add further integration tests
Missing subcommand: upgrade
Be consistent with the number of lines
0 plugins -> empty string (instead of 2 lines)
1 plugin -> 1 line (instead of 2 lines)
2 plugins -> 2 lines (instead of 3 lines)

@corneliusweig corneliusweig force-pushed the corneliusweig:more-integration-tests branch from 31d3f2f to 356a9ee Jun 10, 2019

@k8s-ci-robot k8s-ci-robot added size/L and removed size/XL labels Jun 10, 2019

@corneliusweig corneliusweig force-pushed the corneliusweig:more-integration-tests branch from 8d1a336 to e73978c Jun 10, 2019

@corneliusweig corneliusweig changed the title [WIP] More integration tests More integration tests Jun 10, 2019

Show resolved Hide resolved cmd/krew/cmd/list.go Outdated
if string(in) == "" {
return nil
}
return strings.Split(string(in), "\n")

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jun 10, 2019

Contributor

ideally you should do strings.TrimRight(string(in)) so that you avoid taking trailing \n which almost all commands will have.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jun 10, 2019

Contributor

with this, you don't need the =="" check,

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Jun 10, 2019

Author Contributor

Unfortunately, the =="" is still required. For krew list I need the list of results to grow by one if a plugin is installed. However, when starting from none (resulting in "\n") to one plugin ("whatever\n") would result in 1 entry each.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jun 11, 2019

Contributor

hmm. I'm not sure if that's accurate.

Since in this case the stdout is not a tty, we don't print the tabular output.

I expect no output or at most \n. Both of which would return []string{} (len=0).

When you install a plugin I expect this to return pluginname\n, if you split it by \n it should return []string{"pluginnname"} (len=1).

Alternatively you can also test that case as output.Contains("pluginname"), which is also just fine.

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Jun 11, 2019

Author Contributor

I expect no output or at most \n. Both of which would return []string{} (len=0).

That is unfortunately not the case. If the input string is empty or "\n", it will result in []string{""}.

"ExecutedVersion",
"GitTag",
"GitCommit",
"IndexURI https://github.com/kubernetes-sigs/krew-index.git",

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jun 11, 2019

Contributor

I feel like we'll easily break these spaces. :)
I'm not sure if this whole thing is necessary. I currently only care about exitcode=0. :)

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Jun 11, 2019

Author Contributor

I think it's worth a lot to know if something is present in the output. Do you like a regex-based assertion better?

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jun 11, 2019

Contributor

I think anything is fine.

Show resolved Hide resolved test/krew_test.go Outdated
Test improvements
- clarify logic for `krew list` test
- check error case for `krew info`
- make `krew version` test more resilient against formatting changes
plugin string
shouldErr bool
}{
{

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jun 11, 2019

Contributor

please don't put negative cases to the same test case.
they are different test cases. and these aren't unit tests. tabular tests like []struct{}{} are good for tabular unit tests. but ideally each integration test case should be a separate method testing a particular scenario workflow.

e.g. try

TestKrewInfo
TestKrewInfoInvalidPlugin

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Jun 11, 2019

Author Contributor

Makes sense. The test cases are not split into separate test functions.

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

I recommend removing the WIP label so we can move forward with merging.

@corneliusweig

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

I recommend removing the WIP label so we can move forward with merging.

The WIP label should already have been gone for a while. At least I don't see it any longer.

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link

commented Jun 11, 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:

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 95a9ad7 into kubernetes-sigs:master Jun 11, 2019

2 of 3 checks passed

tide Not mergeable. Needs approved, lgtm labels.
Details
cla/linuxfoundation corneliusweig authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@corneliusweig corneliusweig deleted the corneliusweig:more-integration-tests branch Jun 11, 2019

ahmetb added a commit to ahmetb/krew that referenced this pull request Jun 14, 2019

Make corneliusweig a maintainer
Adding corneliusweig to owners. He has been consistently helping both with
krew and krew-index repositories in terms of:
- developing plugins himself
- taking a stab at krew machinery with large scale code refactors
- adding integration test suite to the project
- adding more validation and test cases
- increasing developer documentation

Some of his notable work:
- kubernetes-sigs#195
- kubernetes-sigs#183
- kubernetes-sigs#191
- kubernetes-sigs#201
- kubernetes-sigs#202
- kubernetes-sigs#203
- kubernetes-sigs#208

He is familiar with the codebase enough to officially review and approve code.

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.