Skip to content

Conversation

@irapandey
Copy link
Contributor

What this PR does / why we need it:
This PR adds unit tests for release note generator to add more coverage to non-covered functions

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

#11368

/area testing
Screenshot 2025-03-07 at 2 35 29 PM

@k8s-ci-robot k8s-ci-robot added area/testing Issues or PRs related to testing cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 7, 2025
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 7, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @irapandey. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

@irapandey
Copy link
Contributor Author

Hey @chandankumar4 - Can you take a look here
Thanks 😄

@chrischdi
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 14, 2025
@chrischdi
Copy link
Member

Please take a look at the linter findings. As soon as they got fixed I'd take a look at this PR :-)

@irapandey irapandey force-pushed the test_case_release_note_list_github branch 2 times, most recently from 6b3dea8 to 8b3ece5 Compare March 29, 2025 14:28
@irapandey
Copy link
Contributor Author

/re-test

@irapandey
Copy link
Contributor Author

Hey @chrischdi

I think the lint task failed due to some error in setup - can we have this re-run so you can review it?

Thanks 😄

@chrischdi
Copy link
Member

👍 Triggered a rerun.

name: "Successful PR Listing",
fields: &githubFromToPRLister{
client: &githubClient{
repo: "kubernetes-sigs/kind",
Copy link
Member

Choose a reason for hiding this comment

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

How about using kubernetes-sigs/cluster-api ?
Probably, to use kind repository doesn't affect in the future, but to use cluster-api is more safety since the repo is itself.

{
name: "Successful PR Listing",
fields: &githubFromToPRLister{
client: &githubClient{
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if we should better use some mock client instead of reaching out to github on unit tests (rate-limiting could apply and make the test flaky)

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @chrischdi
In this case, to reach out to GitHub generates possibility about flaky. For example, when some PR piles the commit in a short period of time, we'll met the rate limit error. Then it's not problem even if we use mock client since we know the result at this time. In the future, using mock might not be problem.

func Test_githubFromToPRLister_listPRs(t *testing.T) {
tests := []struct {
name string
fields *githubFromToPRLister
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fields *githubFromToPRLister
lister *githubFromToPRLister

Copy link
Member

Choose a reason for hiding this comment

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

+1

Comment on lines 185 to 191
l := &githubFromToPRLister{
client: tt.fields.client,
fromRef: tt.fields.fromRef,
toRef: tt.fields.toRef,
branch: tt.fields.branch,
}
_, err := l.listPRs(tt.args)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
l := &githubFromToPRLister{
client: tt.fields.client,
fromRef: tt.fields.fromRef,
toRef: tt.fields.toRef,
branch: tt.fields.branch,
}
_, err := l.listPRs(tt.args)
_, err := tt.lister.listPRs(tt.args)

@sbueringer
Copy link
Member

@irapandey do you have time to address the findings?

@irapandey
Copy link
Contributor Author

@chrischdi - Might have missed the updates here
looking into it immediately

@irapandey irapandey force-pushed the test_case_release_note_list_github branch from 580172b to 321b6c9 Compare May 14, 2025 12:07
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 14, 2025
@irapandey irapandey force-pushed the test_case_release_note_list_github branch from 191ef1d to 833a876 Compare August 4, 2025 09:06
@irapandey irapandey force-pushed the test_case_release_note_list_github branch from 833a876 to af5cc62 Compare August 4, 2025 09:07
@irapandey
Copy link
Contributor Author

Screenshot 2025-08-04 at 3 49 28 PM

@irapandey
Copy link
Contributor Author

Screenshot 2025-08-04 at 3 52 06 PM

@irapandey
Copy link
Contributor Author

Hey @sbueringer

Could you take a look here please 😄

@irapandey irapandey force-pushed the test_case_release_note_list_github branch from cf8c247 to 4b4459f Compare September 4, 2025 16:23
@irapandey irapandey requested a review from chrischdi September 5, 2025 03:52
@chrischdi
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 7, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 31ed7e4fd5afbd303be4baf19f50d364fade4c8d

@sbueringer
Copy link
Member

/assign

@sbueringer
Copy link
Member

Will try to take a look soon

@sbueringer sbueringer added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. area/release Issues or PRs related to releasing labels Oct 9, 2025
@irapandey
Copy link
Contributor Author

Hey @sbueringer - I was on a short break - will make the changes today EOD

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 13, 2025
@sbueringer
Copy link
Member

Thx!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 13, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 77f07cd1886ff4ef306eb9b5b19ad15f91919a21

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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 Oct 13, 2025
@k8s-ci-robot k8s-ci-robot merged commit 0a089bc into kubernetes-sigs:main Oct 13, 2025
18 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.12 milestone Oct 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/release Issues or PRs related to releasing area/testing Issues or PRs related to testing cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants