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.

Sign up for GitHub

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

Jump to bottom

Issue #6006 TestCustomResources with better logging and more precision #6068

Closed
wants to merge 2 commits into from
Closed

Issue #6006 TestCustomResources with better logging and more precision #6068

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ccab99
TestCustomResources with better logging and more precision
coryrc Nov 19, 2019
12067b8
Fatal->Error
coryrc Nov 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 0 additions & 118 deletions test/conformance/api/v1alpha1/resources_test.go
View file
Open in desktop

This file was deleted.

117 changes: 0 additions & 117 deletions test/conformance/api/v1beta1/resources_test.go
View file
Open in desktop

This file was deleted.

50 changes: 29 additions & 21 deletions test/conformance/api/v1/resources_test.go → test/e2e/resources_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package v1
package e2e

import (
"fmt"
Expand All @@ -26,6 +26,7 @@ import (

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
"k8s.io/klog"
pkgTest "knative.dev/pkg/test"
"knative.dev/pkg/test/spoof"
"knative.dev/serving/test"
Expand Down Expand Up @@ -74,8 +75,7 @@ func TestCustomResourcesLimits(t *testing.T) {
}

sendPostRequest := func(resolvableDomain bool, url *url.URL) (*spoof.Response, error) {
t.Logf("Request %s", url)
client, err := pkgTest.NewSpoofingClient(clients.KubeClient, t.Logf, url.Hostname(), resolvableDomain)
client, err := pkgTest.NewSpoofingClient(clients.KubeClient, klog.V(4).Infof, url.Hostname(), resolvableDomain)
if err != nil {
return nil, err
}
Expand All @@ -87,31 +87,39 @@ func TestCustomResourcesLimits(t *testing.T) {
return client.Do(req)
}

pokeCowForMB := func(mb int) error {
bloatAndCheck := func(mb int, wantSuccess bool) {
expect := "failure"
if wantSuccess {
expect = "success"
}
klog.V(2).Infof("Bloating by %d MB and expecting %s.\n", mb, expect)
u, _ := url.Parse(endpoint.String())
q := u.Query()
q.Set("bloat", fmt.Sprintf("%d", mb))
u.RawQuery = q.Encode()
response, err := sendPostRequest(test.ServingFlags.ResolvableDomain, u)
if err != nil {
return err
}
if response.StatusCode != http.StatusOK {
return fmt.Errorf("StatusCode = %d, want %d", response.StatusCode, http.StatusOK)
klog.V(5).Infof("Received error '%+v' from sendPostRequest (may be expected)\n", err)
Copy link

Choose a reason for hiding this comment

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

Not sure I see the value in logging the error if the error is expected. Why not put this down in the "wantSuccess" statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's why this is at trace level, V(5). I found it valuable when it was a conformance test.

if wantSuccess {
t.Fatalf("Didn't get a response from bloating RAM with %d MBs", mb)
coryrc marked this conversation as resolved.
Show resolved Hide resolved
}
} else if response.StatusCode == http.StatusOK {
if !wantSuccess {
t.Fatalf("We shouldn't have got a response from bloating RAM with %d MBs", mb)
coryrc marked this conversation as resolved.
Show resolved Hide resolved
}
} else if response.StatusCode == http.StatusBadRequest {
t.Error("Test Issue: Received BadRequest from test app, which probably means the test & test image are not cooperating with each other.")
Copy link

Choose a reason for hiding this comment

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

This error seems unnecessarily long and should include the request that was sent to make debugging easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think this information should not be provided in the log -- like move the second half to a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As for including request, if people run at verbosity 6 through 9 they can see the request, so is that not sufficient?

} else {
// Accept all other StatusCode as failure; different systems could return 404, 502, etc on failure
klog.V(5).Infof("Received http code '%d' from sendPostRequest; interpreting as failure of bloat\n", response.StatusCode)
if wantSuccess {
t.Fatalf("Didn't get a good response from bloating RAM with %d MBs", mb)
}
}
return nil
}

t.Log("Querying the application to see if the memory limits are enforced.")
if err := pokeCowForMB(100); err != nil {
t.Fatalf("Didn't get a response from bloating cow with %d MBs of Memory: %v", 100, err)
}

if err := pokeCowForMB(200); err != nil {
t.Fatalf("Didn't get a response from bloating cow with %d MBs of Memory: %v", 200, err)
}

if err := pokeCowForMB(500); err == nil {
t.Fatalf("We shouldn't have got a response from bloating cow with %d MBs of Memory: %v", 500, err)
}
klog.V(0).Infoln("Querying the application to see if the memory limits are enforced.")
bloatAndCheck(100, true)
bloatAndCheck(200, true)
bloatAndCheck(500, false)
}