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

Update test for checking last pinned timestamp #5830

Merged
merged 1 commit into from
Jan 16, 2020
Merged

Update test for checking last pinned timestamp #5830

merged 1 commit into from
Jan 16, 2020

Conversation

taragu
Copy link
Contributor

@taragu taragu commented Oct 18, 2019 •
edited
Loading

/lint

Proposed Changes

This PR updates the test TestReconcileTargetRevisions. Currently, the test is only checking if an err is returned (there's no verification on the last pinned timestamp as the TODO indicates).

Release Note

NONE

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 18, 2019
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 18, 2019
knative-prow-robot
knative-prow-robot reviewed Oct 18, 2019
View reviewed changes
Copy link
Contributor

@knative-prow-robot knative-prow-robot left a comment

Choose a reason for hiding this comment

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

@taragu: 0 warnings.

In response to this:

/lint

Proposed Changes

This PR updates the test TestReconcileTargetRevisions. Currently, the test is only checking if an err is returned (there's no verification on the last pinned timestamp as the TODO indicates). Also, the two test cases are actually the same, so this PR reduces the test down to one test case.

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.

@knative-prow-robot knative-prow-robot added area/API API objects and controllers area/networking labels Oct 18, 2019
@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/route/reconcile_resources.go 84.2% 82.7% -1.5

@taragu
Copy link
Contributor Author

taragu commented Oct 18, 2019

/test pull-knative-serving-go-coverage

vagababov
vagababov reviewed Oct 18, 2019
View reviewed changes
Copy link
Contributor

@vagababov vagababov left a comment

Choose a reason for hiding this comment

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

Looks like coverage dropped :-)

@taragu
Copy link
Contributor Author

taragu commented Oct 28, 2019

This PR is ready for another look.

@taragu
Copy link
Contributor Author

taragu commented Nov 12, 2019

@vagababov would you please take a look when you get a chance?

vagababov
vagababov reviewed Jan 8, 2020
View reviewed changes
pkg/reconciler/route/reconcile_resources_test.go Outdated
t.Fatal("The last pinned timestamp is not updated")
}

// Now, try reconciling target revisions for a revision that does not exist. No err should be returned
Copy link
Contributor

Choose a reason for hiding this comment

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

Why? Shouldn't this fail, since there is no revision to reconcile?

@taragu taragu force-pushed the greg-todo branch 2 times, most recently from 132e390 to 41a4196 Compare January 9, 2020 15:50
@taragu taragu force-pushed the greg-todo branch 2 times, most recently from b6029fc to 8f2fe5c Compare January 9, 2020 16:03
@knative-test-reporter-robot
Copy link

The following jobs failed:

Test name Triggers Retries
pull-knative-serving-integration-tests pull-knative-serving-integration-tests 1/3
pull-knative-serving-unit-tests 0/3

Automatically retrying due to test flakiness...
/test pull-knative-serving-integration-tests

@taragu
Copy link
Contributor Author

taragu commented Jan 9, 2020

/retest

vagababov
vagababov reviewed Jan 9, 2020
View reviewed changes
Copy link
Contributor

@vagababov vagababov left a comment

Choose a reason for hiding this comment

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

Nit.
/lgtm
from my side
/assign @dgerd
for approval.

pkg/reconciler/route/reconcile_resources_test.go Outdated
}
}

func TestReconcileTargetDoNotExistRevision(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func TestReconcileTargetDoNotExistRevision(t *testing.T) {
func TestReconcileRevisionTargetDoesNotExist(t *testing.T) {

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 9, 2020
Update test for checking last pinned timestamp
e05abda 
This PR updates the tests for checking last pinned timestamp for both valid revisions and invalid revisions.
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 9, 2020
@taragu
Copy link
Contributor Author

taragu commented Jan 16, 2020

@dgerd would you take a look when you get a chance?

@dgerd
Copy link

dgerd commented Jan 16, 2020

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 16, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgerd, taragu

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 Jan 16, 2020
@knative-prow-robot knative-prow-robot merged commit fe0448b into knative:master Jan 16, 2020
@taragu taragu deleted the greg-todo branch February 18, 2020 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vagababov vagababov vagababov left review comments

@mattmoor mattmoor mattmoor left review comments

@savitaashture savitaashture savitaashture left review comments

@knative-prow-robot knative-prow-robot knative-prow-robot left review comments

@markusthoemmes markusthoemmes Awaiting requested review from markusthoemmes

Assignees

@vagababov vagababov

@dgerd dgerd

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/API API objects and controllers area/networking cla: yes Indicates the PR's author has signed the CLA. 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.
9 participants
@taragu @knative-metrics-robot @knative-test-reporter-robot @dgerd @knative-prow-robot @vagababov @mattmoor @savitaashture @googlebot