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

make TestListPager_EachListItem rework #111241

Merged
merged 1 commit into from Aug 26, 2022
Merged

make TestListPager_EachListItem rework #111241

merged 1 commit into from Aug 26, 2022

Conversation

Abirdcfly
Copy link
Member

Signed-off-by: Abirdcfly fp544037857@gmail.com

What type of PR is this?

/kind bug

What this PR does / why we need it:

TestListPager_EachListItem not work fine.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 19, 2022
@Abirdcfly Abirdcfly mentioned this pull request Jul 19, 2022
Merged
@Abirdcfly
Copy link
Member Author

Abirdcfly commented Jul 19, 2022 •
edited

/hold
I need some advice to determine whether I should fix the tests or the source code.

Here are 2 tests where the results look different than expected:

  1. When calling cancel, should it return exactly the number at the time of cancel (3), or the number of chunks (10)?
  2. The other issue is the number returned when call panic.

@aojea #111228 (comment)
@caesarxuchao #111241 (comment)
@jpbetz #75849

I guess this should also (Please correct me if I am wrong)
/cc @liggitt

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 19, 2022
@aojea
Copy link
Member

aojea commented Jul 19, 2022

not familiar with this code , maybe @wojtek-t or @liggitt ?

@leilajal
Copy link
Contributor

/cc @jpbetz
/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 19, 2022
@Abirdcfly
Copy link
Member Author

/cc @liggitt

The last cc didn't seem to work

@liggitt
Copy link
Member

liggitt commented Aug 4, 2022

I agree the Fatal-or-return logic was wrong and was masking issues with the test

I'll defer to @jpbetz' review of the test changes themselves

@liggitt liggitt removed their request for review August 4, 2022 13:47
@Abirdcfly
Copy link
Member Author

/cc @jpbetz

jpbetz
jpbetz reviewed Aug 5, 2022
View reviewed changes
staging/src/k8s.io/client-go/tools/pager/pager_test.go Outdated
@@ -301,14 +301,14 @@ func TestListPager_EachListItem(t *testing.T) {
{
name: "cancel context while processing",
fields: fields{PageSize: 10, PageFn: (&testPager{t: t, expectPage: 10, remaining: 51, rv: "rv:20"}).PagedList},
want: list(3, "rv:20"), // all the items <= the one the processor returned an error on should have been visited
want: list(10, "rv:20"), // all the items <= the one the processor returned an error on should have been visited
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks right. The whole PageSize worth of items got returned.

jpbetz
jpbetz reviewed Aug 5, 2022
View reviewed changes
staging/src/k8s.io/client-go/tools/pager/pager_test.go Outdated
{
name: "panic processing item",
fields: fields{PageSize: 10, PageFn: (&testPager{t: t, expectPage: 10, remaining: 51, rv: "rv:20"}).PagedList},
want: list(3, "rv:20"), // all the items <= the one the processor returned an error on should have been visited
want: list(51, "rv:20"), // all the items <= the one the processor returned an error on should have been visited
wantPanic: true,
},
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is just plain wrong. It should have had processorPanicOnItem: 3 set. It overlaps with the above error processing item and so should just be removed. My apologies.

jpbetz
jpbetz reviewed Aug 5, 2022
View reviewed changes
staging/src/k8s.io/client-go/tools/pager/pager_test.go
@@ -345,8 +345,7 @@ func TestListPager_EachListItem(t *testing.T) {
err = p.EachListItem(ctx, metav1.ListOptions{}, fn)
}()
if (panic != nil) && !tt.wantPanic {
t.Fatalf(".EachListItem() panic = %v, wantPanic %v", panic, tt.wantPanic)
} else {
t.Errorf(".EachListItem() panic = %v, wantPanic %v", panic, tt.wantPanic)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for catching this.

@Abirdcfly
make TestListPager_EachListItem rework
5e84f6b 
Signed-off-by: Abirdcfly <fp544037857@gmail.com>
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 6, 2022
@Abirdcfly Abirdcfly changed the title fix TestListPager_EachListItem make TestListPager_EachListItem rework Aug 6, 2022
@Abirdcfly
Copy link
Member Author

/assign @caesarxuchao

@Abirdcfly
Copy link
Member Author

/test pull-kubernetes-e2e-gce-ubuntu-containerd

@Abirdcfly
Copy link
Member Author

/unhold

ready for review.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 6, 2022
@jpbetz
Copy link
Contributor

jpbetz commented Aug 8, 2022

/lgtm

Needs approver.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 8, 2022
@aojea
Copy link
Member

aojea commented Aug 8, 2022

/lgtm

Needs approver.

/Assign @lavalamp

@Abirdcfly
Copy link
Member Author

kindly ping @lavalamp @caesarxuchao for approve, because master is open again for 1.26 =.=

@aojea
Copy link
Member

aojea commented Aug 26, 2022

/approve

@aojea
Copy link
Member

aojea commented Aug 26, 2022

/assign @liggitt

for approval,

/lgtm

Needs approver.

@liggitt
Copy link
Member

liggitt commented Aug 26, 2022

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Abirdcfly, aojea, liggitt

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 26, 2022
@k8s-ci-robot k8s-ci-robot merged commit ce2c587 into kubernetes:master Aug 26, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Aug 26, 2022
@enj enj mentioned this pull request Sep 15, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpbetz jpbetz jpbetz left review comments

@soltysh soltysh Awaiting requested review from soltysh

Assignees

@jpbetz jpbetz

@lavalamp lavalamp

@liggitt liggitt

@caesarxuchao caesarxuchao

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.26
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@Abirdcfly @aojea @leilajal @liggitt @jpbetz @k8s-ci-robot @lavalamp @caesarxuchao