Skip to content
This repository was archived by the owner on Sep 5, 2019. It is now read-only.
Merged
5 changes: 4 additions & 1 deletion pkg/builder/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,10 @@ func IsTimeout(status *v1alpha1.BuildStatus, buildTimeout string) bool {
return false
}
}
return time.Since(status.CreationTime.Time).Seconds() > timeout.Seconds()
if status.StartTime.Time.IsZero() {
return false
}
return time.Since(status.StartTime.Time).Seconds() > timeout.Seconds()
}

// ErrorMessage returns the error message from the status.
Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/build/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ func TestBasicFlows(t *testing.T) {
t.Errorf("error fetching build: %v", err)
}
// Update status to current time
first.Status.CreationTime = metav1.Now()
first.Status.StartTime = metav1.Now()

if builder.IsDone(&first.Status) {
t.Errorf("First IsDone(%d); wanted not done, got done.", idx)
Expand Down
60 changes: 60 additions & 0 deletions test/e2e/simple_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,3 +120,63 @@ func TestFailingBuild(t *testing.T) {
t.Fatalf("watchBuild did not return expected error: %v", err)
}
}

func TestBuildLowTimeout(t *testing.T) {
clients := setup(t)

buildName := "build-low-timeout"
if _, err := clients.buildClient.builds.Create(&v1alpha1.Build{
ObjectMeta: metav1.ObjectMeta{
Namespace: buildTestNamespace,
Name: buildName,
},
Spec: v1alpha1.BuildSpec{
Timeout: "20s",
Steps: []corev1.Container{{
Name: "lowtimeoutstep",
Image: "ubuntu",
Command: []string{"/bin/bash"},
Args: []string{"-c", "sleep 2000"},
}},
},
}); err != nil {
t.Fatalf("Error creating build: %v", err)
}

b, err := clients.buildClient.watchBuild(buildName)
if err == nil {
t.Fatalf("watchBuild did not return expected BuildTimeout error")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This could probably be t.Error, and we can keep checking other things about the returned build.

}

// verify reason for build failure is timeout

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we add a check that the build executed for 20s, +/- some wiggle room? That wiggle room will be pretty high today, since we only poll every 30s.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay how about we check if build duration is between buildTimeout and buildTimeout + 40s(30s poll + 10s tolerance)?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah that sounds good. Add a TODO to reduce the wiggle room once we're more strict, and maybe run the test a few times to make sure it's not flaky.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I ran into couple of edge cases while re-running the test. So I fixed couple of bugs in this PR.

if b.Status.GetCondition(v1alpha1.BuildSucceeded).Reason != "BuildTimeout" {
t.Fatalf("wanted BuildTimeout; got %q", b.Status.GetCondition(v1alpha1.BuildSucceeded).Reason)
}
}

func TestBuildWithHighTimeout(t *testing.T) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure you need this test here, since the build is actually successful

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1, our test that a build under the timeout passes is TestSimpleBuild

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree. I wanted to test a build with higher timeout set. As an alternate idea I could alter the SimpleBuild test to have a timeout as well.

clients := setup(t)

buildName := "build-high-timeout"
if _, err := clients.buildClient.builds.Create(&v1alpha1.Build{
ObjectMeta: metav1.ObjectMeta{
Namespace: buildTestNamespace,
Name: buildName,
},
Spec: v1alpha1.BuildSpec{
Timeout: "40s",
Steps: []corev1.Container{{
Name: "hightimeoutstep",
Image: "ubuntu",
Command: []string{"/bin/bash"},
Args: []string{"-c", "sleep 20"},
}},
},
}); err != nil {
t.Fatalf("Error creating build: %v", err)
}

if _, err := clients.buildClient.watchBuild(buildName); err != nil {
t.Fatalf("Did not expect error: %v", err)
}
}