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

Use table tests for revision scaler (#1677) #1693

Merged
merged 3 commits into from
Aug 8, 2018
Merged

Use table tests for revision scaler (#1677) #1693

merged 3 commits into from
Aug 8, 2018

Conversation

tanzeeb
Copy link
Contributor

@tanzeeb tanzeeb commented Jul 25, 2018

Fixes #1677

@google-prow-robot google-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 25, 2018
mattmoor
mattmoor reviewed Jul 26, 2018
View reviewed changes
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

thanks for picking this up

pkg/autoscaler/revision_scaler_test.go Outdated
wantState v1alpha1.RevisionServingStateType
wantReplicas int
}{
{"scales to zero", v1alpha1.RevisionServingStateActive, 1, 0, v1alpha1.RevisionServingStateReserve, 1},
Copy link
Member

Choose a reason for hiding this comment

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

nit: I generally prefer the style of:

{{
  label: "...",
  startState: ...
}, {
  label: "..."
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, will update

pkg/autoscaler/revision_scaler_test.go Outdated
@@ -146,6 +129,6 @@ func checkReplicas(t *testing.T, kubeClient kubernetes.Interface, deployment *v1
}

if *updatedDeployment.Spec.Replicas != int32(expectedScale) {
t.Fatal("Unexpected deployment replicas.", updatedDeployment.Spec.Replicas)
t.Fatal("Unexpected deployment replicas.", *updatedDeployment.Spec.Replicas)
Copy link
Member

Choose a reason for hiding this comment

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

Fatalf?

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 mean, use Fatalf to print a "want x, got y" style message, or use Fatalf to not have to dereference the pointer?

Copy link
Member

Choose a reason for hiding this comment

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

I just meant that seems more natural when passing args.

@mattmoor mattmoor self-assigned this Jul 26, 2018
@google-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tanzeeb
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mattmoor

If they are not already assigned, you can assign the PR to them by writing /assign @mattmoor in a comment when ready.

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:

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

@knative-metrics-robot
Copy link

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to run the coverage report again

File Old Coverage New Coverage Delta
pkg/autoscaler/revision_scaler.go Do not exist 80.0%

@yolocs yolocs mentioned this pull request Aug 1, 2018
Closed
mattmoor
mattmoor reviewed Aug 8, 2018
View reviewed changes
pkg/autoscaler/revision_scaler_test.go Outdated
wantState v1alpha1.RevisionServingStateType
wantReplicas int
}{
{
Copy link
Member

Choose a reason for hiding this comment

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

nit: if you use {{ style it'll drop a lot of indentation.

@mattmoor
Copy link
Member

mattmoor commented Aug 8, 2018

@tanzeeb do you want to get this updated and ping me on slack, so we can get this in?

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@dprotaso dprotaso force-pushed the revision-scaler-table-tests branch from 91f630c to 6a2b27c Compare August 8, 2018 18:40
@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 8, 2018
@dprotaso
Copy link
Member

dprotaso commented Aug 8, 2018

I consent to the CLA

@dprotaso dprotaso force-pushed the revision-scaler-table-tests branch from 6a2b27c to ce0e875 Compare August 8, 2018 18:46
mattmoor
mattmoor approved these changes Aug 8, 2018
View reviewed changes
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 8, 2018
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, tanzeeb

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:

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 8, 2018
@mattmoor
Copy link
Member

mattmoor commented Aug 8, 2018

Will merge manually after integration tests pass due to CLA issues.

@mattmoor mattmoor merged commit 56cceac into knative:master Aug 8, 2018
@glyn
Copy link
Contributor

glyn commented Aug 9, 2018

@tanzeeb thanks very much for doing this - the tests are so much cleaner now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattmoor mattmoor mattmoor approved these changes

@josephburnett josephburnett Awaiting requested review from josephburnett

Assignees

@mattmoor mattmoor

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@tanzeeb @google-prow-robot @knative-metrics-robot @mattmoor @googlebot @dprotaso @knative-prow-robot @glyn