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
extended tests for jenkins openshift V3 plugin #6329
extended tests for jenkins openshift V3 plugin #6329
Conversation
return data | ||
} | ||
|
||
var _ = g.Describe("jenkins: plugin: run job leverging openshift pipeline plugin", func() { |
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.
leveraging
6020f06
to
c0f132f
Compare
@bparees updates based on 1st set of comments |
@@ -189,22 +189,22 @@ var CheckImageStreamTagNotFoundFn = func(i *imageapi.ImageStream) bool { | |||
func WaitForADeployment(client kclient.ReplicationControllerInterface, |
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.
I think i would have done this differently.... either:
- just check the start time and calculate and end time, and then if the end time has passed at the top of the outer loop
- get rid of the the Watch entirely and just wrap the list() in a wait.Poll check (which is basically what WaitForABuild does).
with the existing logic it's pretty hard to follow how long it's really going to end up waiting, based on expiration of watches. also 3 minutes between polls seems like a lot.
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.
went with option 1) ... will push in a sec
c0f132f
to
5170ed4
Compare
//NewApp performs an oc new-app invocation from the supplied json file | ||
func NewApp(jsonFilePath string, oc *CLI) error { | ||
err := oc.Run("new-app").Args(jsonFilePath).Execute() | ||
return err |
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.
just return the oc.Run here :-)
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.
is this worth to have special function? I think reading the code will tell you what this does.... having NewApp() makes you go find the definition if something explode :)
5170ed4
to
a1f4791
Compare
hack/install-assets.sh failed ... never got to the extended tests in the extended test run |
@@ -189,7 +189,9 @@ var CheckImageStreamTagNotFoundFn = func(i *imageapi.ImageStream) bool { | |||
func WaitForADeployment(client kclient.ReplicationControllerInterface, | |||
name string, | |||
isOK, isFailed func(*kapi.ReplicationController) bool) error { | |||
for { | |||
startTime := time.Now() | |||
endTime := startTime.Add(3 * time.Minute) |
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.
bump this up to something like 15 or 20 minutes. we don't want to introduce flakes if we have existing extended tests that take a long time waiting for a deployment for some reason.
i'm also surprised 3 minutes is long enough for you test, given that it basically has to get through the entire imagebuild+deploy step in 3 minutes to pass.
(3 minutes or the watch timeout, whichever is longer.)
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.
Will do - it worked for me locally but i did not measure how long it
typically took
On Wednesday, December 16, 2015, Ben Parees notifications@github.com
wrote:
In test/extended/util/framework.go
#6329 (comment):@@ -189,7 +189,9 @@ var CheckImageStreamTagNotFoundFn = func(i _imageapi.ImageStream) bool {
func WaitForADeployment(client kclient.ReplicationControllerInterface,
name string,
isOK, isFailed func(_kapi.ReplicationController) bool) error {
- for {
- startTime := time.Now()
- endTime := startTime.Add(3 * time.Minute)
bump this up to something like 15 or 20 minutes. we don't want to
introduce flakes if we have existing extended tests that take a long time
waiting for a deployment for some reason.i'm also surprised 3 minutes is long enough for you test, given that it
basically has to get through the entire imagebuild+deploy step in 3 minutes
to pass.(3 minutes or the watch timeout, whichever is longer.)
—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/6329/files#r47846861.
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.
I'm guessing the watch doesn't get closed because there's not much etcd
change log activity, so you don't come back to the top of the loop, so the
3 minute limit doesn't come into play in your particular scenario.
Ben Parees | OpenShift
On Dec 16, 2015 10:20 PM, "Gabe Montero" notifications@github.com wrote:
In test/extended/util/framework.go
#6329 (comment):@@ -189,7 +189,9 @@ var CheckImageStreamTagNotFoundFn = func(i _imageapi.ImageStream) bool {
func WaitForADeployment(client kclient.ReplicationControllerInterface,
name string,
isOK, isFailed func(_kapi.ReplicationController) bool) error {
- for {
- startTime := time.Now()
- endTime := startTime.Add(3 * time.Minute)
Will do - it worked for me locally but i did not measure how long it
typically took
… <#151adf41a51c75cf_>
On Wednesday, December 16, 2015, Ben Parees notifications@github.com
wrote: In test/extended/util/framework.go <#6329 (comment)
https://github.com/openshift/origin/pull/6329#discussion_r47846861>: >
@@ -189,7 +189,9 @@ var CheckImageStreamTagNotFoundFn = func(i
_imageapi.ImageStream) bool { > func WaitForADeployment(client
kclient.ReplicationControllerInterface, > name string, > isOK, isFailed
func(_kapi.ReplicationController) bool) error { > - for { > + startTime :=
time.Now() > + endTime := startTime.Add(3 * time.Minute) bump this up to
something like 15 or 20 minutes. we don't want to introduce flakes if we
have existing extended tests that take a long time waiting for a deployment
for some reason. i'm also surprised 3 minutes is long enough for you test,
given that it basically has to get through the entire imagebuild+deploy
step in 3 minutes to pass. (3 minutes or the watch timeout, whichever is
longer.) — Reply to this email directly or view it on GitHub <
https://github.com/openshift/origin/pull/6329/files#r47846861>.—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/6329/files#r47865373.
2b5af4b
to
870339a
Compare
the test only extended keep failing in a early, just after build-go.sh completes successfully, in a weird way, way before it gets to the extended test. tried refreshing the branch; if still fails, going to remove the existing test flags and just try a generic test job. |
ok ... it looks like it got to try and run the extended test, but it seems to fail immediately with an error that is not obvious to me at first blush: Running TEST_REPORT_DIR=/tmp/openshift-extended-tests/junit/extended test/extended/core.sh --ginkgo.focus="jenkins"... I'm going to see about clearing all the prior test flags, see if I can get full fledged test going. |
@gabemontero why do you import "github.com/docker/docker/pkg/units" ? |
OK this seems like kubernetes imports that package... that is weird, do you have godeps setup properly? |
@mfojtik I don't import "github.com/docker/docker/pkg/units" .... as far as i know i do, I did not make any godeps changes as part of this PR ... in any event, wouldn't the test job on jenkins be the one that pulls the various godeps? |
@gabemontero I just ran that command and it worked for me, that means that you might have something wrong with your environment... can you check the value of GOPATH? |
lgtm, waiting for extended test framework to be fixed up. |
870339a
to
4b2f077
Compare
[testonlyextended][extended:core(jenkins)] |
4b2f077
to
5b9ee55
Compare
back to testing, having pulled in pr #6558 while it is waiting to merge, and uncovered a timing issue panic in the wait for jenkins to come up logic ... pushing an update for retest shortly. |
5b9ee55
to
e592ac4
Compare
ok ... the new extended test just passed !! ... Going to push the back out of the 6558 fix, confirm things are still good |
e592ac4
to
d23a24f
Compare
OK that's two straight successful extended test runs. And the travis hiccup appears to be some sort of go 1.4 flake (it works in 1.5). @bparees IMO - this is ready to hit the merge button on - PTAL at your convenience / discretion |
} | ||
} | ||
|
||
// as g.It's are added conceivably will be with new jenkins job def's |
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.
scrambled godoc
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.
or scrambled comment. not sure what it was intended to be.
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.
just deleted - no longer pertinent as I recall
one nit and a question about the deployment that's being verified. otherwise lgtm. |
d23a24f
to
9f1765b
Compare
ok ... i've pushed the updates in response to the one nit and question |
lgtm [merge] |
[Test]ing while waiting on the merge queue |
the https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/8311/ failure came from the https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4555/ failure came from can we re-hit the merge button or force the merge of this new extended test (which passed here: https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/8309/) ? |
i can keep hitting it, but until we sort out the underlying etcd flake issue, don't hold your breath (I don't want to force merge even though this would be low risk, it's just a bad pattern to get into. we need to fix the merge queue) |
@bparees roger that |
9f1765b
to
8cb273b
Compare
[merge] |
[test] |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4584/) (Image: devenv-rhel7_3110) |
Gofmt, not a flake |
8cb273b
to
2b35c51
Compare
Evaluated for origin testonlyextended up to 2b35c51 |
Evaluated for origin test up to 2b35c51 |
yep, found 1 more thing to fix last night, failed to gofmt ... just pushed if needed somebody please hit merge button when get the chance - thx |
[merge] |
Evaluated for origin merge up to 2b35c51 |
continuous-integration/openshift-jenkins/testonlyextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/8413/) (Extended Tests: core(jenkins)) |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/8414/) (Extended Tests: core(jenkins)) |
@bparees @mfojtik PTAL