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

Move test integration #153

Merged
merged 1 commit into from
Dec 24, 2018

Conversation

kschumy
Copy link
Contributor

@kschumy kschumy commented Dec 20, 2018

Is this a bug fix or adding new feature?
Feature

What is this PR about? / Why do we need it?
make test-integration uses aws-k8s-tester

What testing is done?
Ran locally

@gyuho @leakingtapan

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 20, 2018
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 20, 2018
@k8s-ci-robot
Copy link
Contributor

Hi @kschumy. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 20, 2018
@gyuho
Copy link
Member

gyuho commented Dec 20, 2018

/hold

@leakingtapan

As we discussed, we are introducing EC2 based integration tests.

We will test this PR locally.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 20, 2018
@coveralls
Copy link

coveralls commented Dec 20, 2018

Pull Request Test Coverage Report for Build 281

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 50.352%

Totals Coverage Status
Change from base Build 272: 0.0%
Covered Lines: 572
Relevant Lines: 1136

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 247

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 50.352%

Totals Coverage Status
Change from base Build 241: 0.0%
Covered Lines: 572
Relevant Lines: 1136

💛 - Coveralls

@k8s-ci-robot
Copy link
Contributor

@kschumy: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@kschumy kschumy force-pushed the move-test-integration branch 2 times, most recently from edfca00 to 23db543 Compare December 20, 2018 03:28
@leakingtapan
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 20, 2018
Copy link
Contributor

@leakingtapan leakingtapan left a comment

Choose a reason for hiding this comment

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

Could you add some README (maybe under tests/integration) about how to run the integration test locally and what are required for integration test to run, etc?

And lets squash the commit once all the changes are done


if ! [[ "$0" =~ hack/test-integration.sh ]]; then
echo "must be run from repository root"
exit 255
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we use 255 as exit code? Here is the list of special meaning of exit code: http://tldp.org/LDP/abs/html/exitcodes.html Maybe we can use 1 as general error

Copy link
Member

Choose a reason for hiding this comment

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

Yeah let's use exit 127 for illegal command, instead.

/cc @kschumy

@k8s-ci-robot
Copy link
Contributor

@gyuho: GitHub didn't allow me to request PR reviews from the following users: kschumy.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Yeah let's use exit 127 for illegal command, instead.

/cc @kschumy

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

fi

go test -c ./tests/integration/... -o bin/integration.test && \
sudo -E bin/integration.test -ginkgo.v
Copy link
Member

Choose a reason for hiding this comment

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

Ok let's instead do:

sudo -E bin/integration.test -test.v -ginkgo.v

@kschumy kschumy force-pushed the move-test-integration branch 2 times, most recently from 871122f to 036cffe Compare December 21, 2018 05:31
Copy link
Contributor Author

@kschumy kschumy left a comment

Choose a reason for hiding this comment

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

@leakingtapan was this what you wanted?

036cffe

@leakingtapan
Copy link
Contributor

leakingtapan commented Dec 21, 2018

@leakingtapan was this what you wanted?
036cffe

Looks like a good start

@kschumy kschumy force-pushed the move-test-integration branch 3 times, most recently from feb3211 to dc0140c Compare December 22, 2018 02:04
#### Using `aws-ebs-csi-driver`

```bash
cd ${GOPATH}/src/github.com/aws/aws-k8s-tester
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since make test-integration now runs aws-k8s-tester automatically, I changed this to being about running aws-k8s-tester independently

tests/integration/README.md Outdated Show resolved Hide resolved
tests/integration/README.md Outdated Show resolved Hide resolved
@@ -0,0 +1,33 @@
## Running Integration Tests with `aws-k8s-tester`

[`aws-k8s-tester`](https://github.com/aws/aws-k8s-tester) is an e2e test deployment interface for testing K8s clusters on AWS and is used in conjunction with `aws-ebs-csi-driver` for csi integration tests.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

tests/integration/README.md Outdated Show resolved Hide resolved
tests/integration/README.md Outdated Show resolved Hide resolved
@leakingtapan
Copy link
Contributor

Add some comments on the README for integ test. Could you squash all the changes into one commit after all the change?

@leakingtapan leakingtapan mentioned this pull request Dec 22, 2018
5 tasks
Makefile Outdated Show resolved Hide resolved
@kschumy kschumy force-pushed the move-test-integration branch 2 times, most recently from 261aed7 to 9957ffd Compare December 24, 2018 22:07
@gyuho
Copy link
Member

gyuho commented Dec 24, 2018

/lgtm

Defer to @leakingtapan.

In the meantime, @kschumy will be adding a Prow job in upstream test-infra.

Thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 24, 2018
hack: create test-integration.sh

Makefile: use aws-k8s-tester with test-integration

tests/integration: create README
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 24, 2018
@gyuho
Copy link
Member

gyuho commented Dec 24, 2018

/lgtm

thanks for picking up the latest!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 24, 2018
@leakingtapan
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kschumy, leakingtapan

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 24, 2018
@gyuho
Copy link
Member

gyuho commented Dec 24, 2018

/hold cancel

@kschumy will look into ways to automate release version fetch, in the following PR.

Thanks!

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 24, 2018
@k8s-ci-robot k8s-ci-robot merged commit e25854c into kubernetes-sigs:master Dec 24, 2018
@kschumy kschumy deleted the move-test-integration branch December 26, 2018 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants