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

specify flags to test-integration #33952

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Oct 3, 2016

Allows a specific test to be run in test-integration: hack/test-integration.sh auth -test.run=TestKindAuthorization

@eparis I don't know how good or bad my bash is.


This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit 08d41ed. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@eparis
Copy link
Contributor

eparis commented Oct 3, 2016

I doubt the failure on gce e2e is this PRs:

�[0;33mAttempt 5 failed to create firewall rule e2e-gce-agent-pr-29-0-minion-all. Retrying.�[0m
ERROR: (gcloud.compute.firewall-rules.create) Some requests did not succeed:
 - Quota 'FIREWALLS' exceeded. Limit: 200.0

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Oct 3, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit 08d41ed. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@@ -90,8 +90,17 @@ if [[ -n "${KUBE_TEST_ARGS}" ]]; then
runTests v1
fi

# Pass arguments that with "-" and move them to goflags.
Copy link
Contributor

@eparis eparis Oct 3, 2016

Choose a reason for hiding this comment

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

missing word: begining?

what_flags=""
for arg in "$@"; do
if [[ "${arg}" == -* ]]; then
what_flags="${what_flags} \"${arg}\""
Copy link
Contributor

Choose a reason for hiding this comment

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

what_flags=()
for arg in "$@"; do
  if [[ "${arg}" == -* ]]; then
    what_flags+=("${arg}")
  fi
done

echo "${what_flags[@]}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

echo "${what_flags[@]}"

Where do I do this echo? Just informational or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what_flags=()
for arg in "$@"; do
if [[ "${arg}" == -* ]]; then
what_flags+=("${arg}")
fi
done

echo "${what_flags[@]}"

Didn't work. It dropped the second flag, so hack/test-integration.sh auth -test.run=TestKindAuthorization -v didn't give me the -v.

@deads2k deads2k force-pushed the annoyance-specific-integration-test branch from 08d41ed to a134620 Compare October 4, 2016 12:45
@deads2k
Copy link
Contributor Author

deads2k commented Oct 4, 2016

@stevekuznetsov can you help with this bash?

@deads2k deads2k added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Oct 4, 2016
@@ -62,7 +62,7 @@ runTests() {
# TODO: Re-enable race detection when we switch to a thread-safe etcd client
# KUBE_RACE="-race"
make -C "${KUBE_ROOT}" test \
WHAT="$(kube::test::find_integration_test_dirs ${2-} | paste -sd' ' -)" \
WHAT="$(kube::test::find_integration_test_dirs ${2-} | paste -sd' ' -) ${3-} " \
Copy link
Contributor

Choose a reason for hiding this comment

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

We prefer ${var:-'default'} since it's not often that someone wants to pass an null var around as a meaningful value.

@@ -90,8 +90,17 @@ if [[ -n "${KUBE_TEST_ARGS}" ]]; then
runTests v1
fi

# Pass arguments that begin with "-" and move them to goflags.
Copy link
Contributor

Choose a reason for hiding this comment

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

You may want optparse for parsing options ... not sure attempts to do things like this are a good idea, they can get bloated quickly and have rough edges.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You may want optparse for parsing options ... not sure attempts to do things like this are a good idea, they can get bloated quickly and have rough edges.

Not for this I don't think.

fi
done


# Convert the CSV to an array of API versions to test
IFS=';' read -a apiVersions <<< "${KUBE_TEST_API_VERSIONS}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm 99% sure this IFS reset is leaking to the rest of the script

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm 99% sure this IFS reset is leaking to the rest of the script

preexisting

Copy link
Contributor

Choose a reason for hiding this comment

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

Leave it better than you found it and all that :)

# Convert the CSV to an array of API versions to test
IFS=';' read -a apiVersions <<< "${KUBE_TEST_API_VERSIONS}"
for apiVersion in "${apiVersions[@]}"; do
runTests "${apiVersion}" "${1-}"
runTests "${apiVersion}" "${1-}" "${what_flags}"
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work how you think it will. You're always going to have one argument to runTests that you break up later, but it will break like this:

myvar="\"-first\" \"-second=val\" \"-third 'other val'\""
...
runTests "${apiVersion}" "${1-}" "\"-first\" \"-second=val\" \"-third 'other val'\""
         ^----arg0-----^ ^-arg1^ ^----------------------arg2-----------------------^
...
make
    ...
    WHAT="... \"-first\" \"-second=val\" \"-third 'other val'\""
              ^--arg0--^ ^----arg1-----^ ^-arg2-^ ^arg3^ ^arg4^

What you want to do is:

  • make $what_flags into an array
  • append to the array correctly (array+=( "new item with spaces or whatever" ))
  • expand the array into args here runTests ... "${what_flags[@]}"
  • expect varargs in runTests from arg 3 onward WHAT="$( whatever ) ${@:4}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What you want to do is:

make $what_flags into an array
append to the array correctly (array+=( "new item with spaces or whatever" ))
expand the array into args here runTests ... "${what_flags[@]}"
expect varargs in runTests from arg 3 onward WHAT="$( whatever ) ${@:4}"

Doesn't work.

@deads2k deads2k force-pushed the annoyance-specific-integration-test branch from a134620 to 48c1e83 Compare October 4, 2016 18:16
@deads2k
Copy link
Contributor Author

deads2k commented Oct 4, 2016

Alright, pretty sure I got everyone's bash nits. You still can't read it.

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit 48c1e83. Full PR test history.

The magic incantation to run this job again is @k8s-bot gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@deads2k deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 5, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 0ad50d2 into kubernetes:master Oct 5, 2016
@deads2k deads2k deleted the annoyance-specific-integration-test branch February 1, 2017 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants