Refactor test stages into functions. #2164

josephburnett · 2018-10-05T17:50:27Z

Proposed Changes

Refactor autoscale E2E into stages in preparation for adding more tests.

E.g. the next test I will create will be something like:

	ctx := setup(t)
	defer tearDown(ctx)
	stopChan := DiagnoseMeEvery(15*time.Second, ctx.clients, ctx.logger)
	defer close(stopChan)

	assertScaleDown(ctx)
	bounceController(ctx)
	assertScaleUp(ctx)

Release Note

NONE

knative-prow-robot

@josephburnett: 2 warnings.

In response to this:

Proposed Changes

Refactor autoscale E2E into stages in preparation for adding more tests.

E.g. the next test I will create will be something like:
  ctx := setup(t)
  defer tearDown(ctx)
  stopChan := DiagnoseMeEvery(15*time.Second, ctx.clients, ctx.logger)
  defer close(stopChan)

  assertScaleDown(ctx)
  bounceController(ctx)
  assertScaleUp(ctx)
Release Note
NONE

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

test/e2e/autoscale_test.go

josephburnett · 2018-10-05T22:22:57Z

/lint

knative-prow-robot

@josephburnett: 0 warnings.

In response to this:

/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vdemeester

Few comments 👼

vdemeester · 2018-10-08T07:46:02Z

test/e2e/autoscale_test.go

-	logger.Infof("All %d requests succeeded.", totalRequests)
-	return nil
+type testContext struct {
+	t              *testing.T


Not a huge fan of having testing.T inside the testContext… Especially as it doesn't seem to be used in the assert function (doing a logger.Fatal instead)

I should be doing ctx.t.Fatalf instead to fail the test.

vdemeester · 2018-10-08T07:47:10Z

test/e2e/autoscale_test.go

+	return nil
+}
+
+func TestAutoscaleUpDownUp(t *testing.T) {


ctx := setup(t) stopChan := DiagnoseMeEvery(15*time.Second, ctx.clients, ctx.logger) defer close(stopChan) defer tearDown(t, ctx) assertScaleUp(t, ctx) assertScaleDown(t, ctx) assertScaleUp(t, ctx)

I put the defer tearDown after the setup in case the DiagnoseMeEvery call panics.

vdemeester

/approve

adrcunha · 2018-10-11T05:27:13Z

test/e2e/autoscale_test.go

+			if err != nil {
+				return fmt.Errorf("error creating spoofing client: %v", err)
+			}
+			// ctx.logger.Infof( /* DO NOT SUBMIT */ "sending request to %v", fmt.Sprintf("http://%s", ctx.domain))


Remove dead code?

adrcunha · 2018-10-11T05:30:28Z

test/e2e/autoscale_test.go

-func setup(t *testing.T, logger *logging.BaseLogger) *test.Clients {
+func setup(t *testing.T) *testContext {
+	//add test case specific name to its own logger
+	logger := logging.GetContextLogger("TestAutoscaleUpDownUp")


I believe you meant to use t.Name() here, instead of the hardcoded test name.

adrcunha · 2018-10-11T05:34:41Z

You have a conflict that needs to be resolved, so my /lgtm won't do any good. Ping me once you get it done and I'll /lgtm.

adrcunha · 2018-10-17T17:07:01Z

Build errors :(

W1017 16:59:50.535] # github.com/knative/serving/test/e2e
W1017 16:59:50.535] test/e2e/autoscale_test.go:124:1: missing return at end of function
W1017 16:59:50.535] test/e2e/autoscale_test.go:209:24: not enough arguments in call to generateTraffic
W1017 16:59:50.536] 	have (*testContext, number, time.Duration)
W1017 16:59:50.536] 	want (*"github.com/knative/serving/test".Clients, *logging.BaseLogger, int, time.Duration, string)
W1017 16:59:50.536] test/e2e/autoscale_test.go:258:12: undefined: group
W1017 16:59:50.536] test/e2e/autoscale_test.go:259:3: too many arguments to return
W1017 16:59:50.536] 	have (error)
W1017 16:59:50.537] 	want ()
W1017 16:59:50.537] test/e2e/autoscale_test.go:262:5: undefined: successfulRequests
W1017 16:59:50.537] test/e2e/autoscale_test.go:262:27: undefined: totalRequests
W1017 16:59:50.537] test/e2e/autoscale_test.go:263:3: too many arguments to return
W1017 16:59:50.537] 	have (error)
W1017 16:59:50.538] 	want ()
W1017 16:59:50.538] test/e2e/autoscale_test.go:264:4: undefined: successfulRequests
W1017 16:59:50.538] test/e2e/autoscale_test.go:264:24: undefined: totalRequests
W1017 16:59:50.538] test/e2e/autoscale_test.go:267:49: undefined: totalRequests
W1017 16:59:50.538] test/e2e/autoscale_test.go:267:49: too many errors

adrcunha · 2018-10-17T17:35:17Z

/lgtm

adrcunha · 2018-10-17T18:01:22Z

/approve

knative-prow-robot · 2018-10-17T18:01:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrcunha, josephburnett, vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~test/OWNERS~~ [adrcunha]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Refactor test stages into functions.

9799f5b

knative-prow-robot requested review from srinivashegde86 and steuhs October 5, 2018 17:50

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 5, 2018

knative-prow-robot reviewed Oct 5, 2018

View reviewed changes

test/e2e/autoscale_test.go Outdated Show resolved Hide resolved

test/e2e/autoscale_test.go Outdated Show resolved Hide resolved

josephburnett added this to the Scaling: Robust E2E Autoscaling Tests milestone Oct 5, 2018

josephburnett requested a review from adrcunha October 5, 2018 17:52

josephburnett assigned adrcunha Oct 5, 2018

Lint fixes.

bbaa0a9

knative-prow-robot reviewed Oct 5, 2018

View reviewed changes

vdemeester reviewed Oct 8, 2018

View reviewed changes

Replace logger.Fatalf with t.Fatalf.

1bd85b2

josephburnett assigned srinivashegde86 and unassigned adrcunha Oct 8, 2018

vdemeester approved these changes Oct 9, 2018

View reviewed changes

srinivashegde86 mentioned this pull request Oct 9, 2018

E2E test restarting Autoscaler and Activator #2170

Closed

adrcunha reviewed Oct 11, 2018

View reviewed changes

josephburnett added 2 commits October 17, 2018 12:56

Merge branch 'master' into e2estages

322faa0

Remove hardcoded test name.

cdfecaa

Fix bad merge.

8ee7b59

knative-prow-robot assigned adrcunha Oct 17, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2018

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 17, 2018

knative-prow-robot merged commit d841ed1 into knative:master Oct 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor test stages into functions. #2164

Refactor test stages into functions. #2164

josephburnett commented Oct 5, 2018

knative-prow-robot left a comment

josephburnett commented Oct 5, 2018

knative-prow-robot left a comment

vdemeester left a comment

vdemeester Oct 8, 2018

josephburnett Oct 8, 2018

vdemeester Oct 8, 2018

josephburnett Oct 8, 2018

vdemeester left a comment

adrcunha Oct 11, 2018

josephburnett Oct 17, 2018

adrcunha Oct 11, 2018

josephburnett Oct 17, 2018

adrcunha commented Oct 11, 2018

adrcunha commented Oct 17, 2018

adrcunha commented Oct 17, 2018

adrcunha commented Oct 17, 2018

knative-prow-robot commented Oct 17, 2018

Refactor test stages into functions. #2164

Refactor test stages into functions. #2164

Conversation

josephburnett commented Oct 5, 2018

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

josephburnett commented Oct 5, 2018

knative-prow-robot left a comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrcunha commented Oct 11, 2018

adrcunha commented Oct 17, 2018

adrcunha commented Oct 17, 2018

adrcunha commented Oct 17, 2018

knative-prow-robot commented Oct 17, 2018