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

Add basic integration tests #203

Merged
merged 14 commits into from Jun 10, 2019

Conversation

Projects
None yet
3 participants
@corneliusweig
Copy link
Contributor

commented Jun 5, 2019

This is the beginning of integration tests for krew. It's in early stages and more tests need to be added, but the general look and feel should be clear. Therefore I'm eager to get some feedback what should be improved. In particular, there's some aggressive caching of the krew-index clone.

Additionally, I haven't thought about how these tests are run on Travis. That needs to be sorted out as well.

validPlugin = "konfig"
)

func TestInstall(t *testing.T) {

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jun 5, 2019

Contributor

Hmm before these, maybe we should try more fundamental things like exit codes, -h/—help working, unknown subcmds failing, Wdyt? I can go ahead and merge this, and iterate later as well.

krewTest.WithIndex().Cmd("install", validPlugin).RunOrFailOutput()
krewTest.Cmd("remove", validPlugin).RunOrFailOutput()

indexFiles, err := krewTest.TempDir().List("store")

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jun 5, 2019

Contributor

I think this is fundamentally testing the wrong thing. It’s testing the implementation detail and not the user contract.

Once you uninstall a plugin these should happen and we should test them instead.

  1. Kubectl-foo no longer in PATH
  2. Krew list shows it as uninstalled
  3. Krew info fails

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Jun 6, 2019

Author Contributor

You are totally right here.

Show resolved Hide resolved integration/integration_test.go Outdated
Show resolved Hide resolved integration/integration_test.go Outdated
Show resolved Hide resolved integration/krew/index.go Outdated
Show resolved Hide resolved integration/integration_test.go Outdated
Show resolved Hide resolved integration/krew/krew.go Outdated
Show resolved Hide resolved integration/krew/krew.go Outdated
Show resolved Hide resolved integration/krew/krew.go Outdated
@ahmetb

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

Ideally next time please send a much smaller PR (maybe 1 test case) but focus on how we structure the test and how we integrate to travis. That provides a baseline with code that is already used, so we can iterate on it. 😊

@corneliusweig

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

@ahmetb Thanks for your feedback, that's exactly what I was hoping for. I think for going forward, I will remove the flawed test cases and add the very basic krew help case. Then let's get this to run this on travis.

More test cases should follow in a separate PR. That should be straightforward to write down when everything else is set up.

corneliusweig added some commits Jun 4, 2019

Add utilities to initialize the krew index
For faster tests, the index is only cloned once per test run.
Additionally, the index is peristently cached between test runs.
Setup PATH variable in tests so that installed plugins can be executed
Also, optionally link to a krew binary as specified via the KREW_BINARY env varible.

@corneliusweig corneliusweig force-pushed the corneliusweig:integration-test branch from 7ae85af to 234c00b Jun 6, 2019

@corneliusweig corneliusweig force-pushed the corneliusweig:integration-test branch from 234c00b to 14c10ad Jun 6, 2019

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

@corneliusweig

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

Note: marking as ready for review to enable travis, but this is still WIP.

Show resolved Hide resolved integration/integration_test.go Outdated

// List lists all the files in the subpath of the temp directory.
// The input path is expected to use '/' as directory separator regardless of the host OS.
func (td *TempDir) List(path string) ([]string, error) {

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jun 6, 2019

Contributor

seems redundant now. feel free to remove.

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

/approve
Looks like once you add a kubectl to the travis env we're good.

corneliusweig added some commits Jun 6, 2019

Move IndexURI to constants
This is important, because importing the URI in integration tests
triggers the init functions in the cmd package. This has very nasty
and hard to find side-effects.
Review comments
- rename package integration -> test
- disable integration tests in run-tests.sh
- remove obsolete tempdir helper
Enable integration tests on travis
- Use a script run-integration-tests.sh for that
- Add instructions how to run this script locally

@corneliusweig corneliusweig force-pushed the corneliusweig:integration-test branch from c475327 to bb48224 Jun 6, 2019

@corneliusweig corneliusweig changed the title [WIP] Add integration tests Add basic integration tests Jun 6, 2019

@corneliusweig

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

@ahmetb There was a little more work needed. But now the integration tests run on travis 🎉 🎊

@ahmetb

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

moving this forward.
/lgtm
/approve

will open an issue about removing install_kubectl_if_needed.

@k8s-ci-robot

This comment has been minimized.

Copy link

commented Jun 10, 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 04288cb into kubernetes-sigs:master Jun 10, 2019

2 of 3 checks passed

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

@corneliusweig corneliusweig deleted the corneliusweig:integration-test branch Jun 10, 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>

ahmetb added a commit that referenced this pull request Jun 18, 2019

Make corneliusweig a maintainer (#214)
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:
- #195
- #183
- #191
- #201
- #202
- #203
- #208

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

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

@corneliusweig corneliusweig referenced this pull request Jun 19, 2019

Open

REQUEST: New membership for corneliusweig #929

5 of 6 tasks complete
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.