-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Adding extended tests for buildconfig postCommit #12047
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
Adding extended tests for buildconfig postCommit #12047
Conversation
[testextended][extended:core(testing build configuration hooks)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also can we remove the existing tests in test/cmd/builds.sh? they seem redundant since you're doing patch here.
oc.Run("create").Args("-f", buildFixture).Execute() | ||
|
||
g.By("waiting for istag to initialize") | ||
wait.Poll(time.Second, 60*time.Second, func() (bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use WaitForAnImageStreamTag()
err := oc.Run("patch").Args("bc/busybox", "-p", "{\"spec\":{\"postCommit\":{\"script\":\"false\",\"command\":null}}}").Execute() | ||
o.Expect(err).NotTo(o.HaveOccurred()) | ||
br, _ := exutil.StartBuildAndWait(oc, "busybox") | ||
br.AssertFailure() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs to confirm the failure was due to the post commit hook running and failing.
err := oc.Run("patch").Args("bc/busybox", "-p", "{\"spec\":{\"postCommit\":{\"command\":[\"sh\",\"-c\"],\"args\":[\"false\"],\"script\":\"\"}}}").Execute() | ||
o.Expect(err).NotTo(o.HaveOccurred()) | ||
br, _ := exutil.StartBuildAndWait(oc, "busybox") | ||
br.AssertFailure() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
err := oc.Run("patch").Args("bc/busybox", "-p", "{\"spec\":{\"postCommit\":{\"args\":[\"false\"],\"command\":null,\"script\":\"\"}}}").Execute() | ||
o.Expect(err).NotTo(o.HaveOccurred()) | ||
br, _ := exutil.StartBuildAndWait(oc, "busybox") | ||
br.AssertFailure() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
@bparees I didn't want to remove them without discussion since having faster lightweight tests is nice for unit testing. But all they do, in effect, is validate schema & oc-patch. Very low value -- so I'll remove them in the next push unless I hear otherwise. |
yeah, let's get rid of them. We need to start culling our tests to get rid of overlap and this feels like low hanging fruit. |
Evaluated for origin testextended up to c74d91c |
continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/841/) (Base Commit: 2eb8b82) (Extended Tests: core(testing build configuration hooks)) |
@bparees Tests are passing. ptal. |
lgtm [merge] |
[Test]ing while waiting on the merge queue |
Evaluated for origin test up to c74d91c |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11786/) (Base Commit: f966ae2) |
flake #12072
[merge]
…On Tue, Nov 29, 2016 at 11:53 PM, OpenShift Bot ***@***.***> wrote:
continuous-integration/openshift-jenkins/merge FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11799/) (Base
Commit: ae9c732
<ae9c732>
)
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#12047 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEvl3kiAgT5jw2f59F3eF9VcQIBELuFEks5rDQFKgaJpZM4K-PKx>
.
--
Ben Parees | OpenShift
|
Evaluated for origin merge up to c74d91c |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11800/) (Base Commit: ae9c732) (Image: devenv-rhel7_5441) |
https://trello.com/c/PfNxCWs9/990-3-extended-tests-for-build-hooks-techdebt
Should have been a 1. You were right, @bparees!