Increase test coverage for groups/ #2873

MadhavJivrajani · 2021-10-04T08:07:50Z

This PR adds fake clients to test out code in groups/

TODO:

Fixes #420
/area groups

k8s-ci-robot · 2021-10-04T08:07:51Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

MadhavJivrajani · 2021-10-04T08:08:23Z

/test all

MadhavJivrajani · 2021-10-04T08:09:25Z

/assign @spiffxp
I'm hoping to make progress on this this week - in case you have early feedback, please lmk! :)

MadhavJivrajani · 2021-10-12T06:56:58Z

@spiffxp @dims while writing tests - I'm a little confused regarding the logic around creating/updating groups:

k8s.io/groups/service.go

Lines 187 to 209 in 75c0411

    
           	if group.Name != "" && grp.Name != group.Name || 
        
           		group.Description != "" && grp.Description != group.Description { 
        
           		if !config.ConfirmChanges { 
        
           			log.Printf("dry-run: would update group name/description %q\n", group.EmailId) 
        
           		} else { 
        
           			log.Printf("Trying to update group: %q\n", group.EmailId) 
        
           			g := admin.Group{ 
        
           				Email: group.EmailId, 
        
           			} 
        
           			if group.Name != "" { 
        
           				g.Name = group.Name 
        
           			} 
        
           			if group.Description != "" { 
        
           				g.Description = group.Description 
        
           			} 
        
           			g4, err := as.client.UpdateGroup(group.EmailId, &g) 
        
           			if err != nil { 
        
           				return fmt.Errorf("unable to update group %q: %w", group.EmailId, err) 
        
           			} 
        
           			log.Printf("> Successfully updated group %s\n", g4.Email) 
        
           		} 
        
           	} 
        
           }

Here, in the top most if condition itself it is checked that Name and Description are non-empty and then again in the if condition it is checked. Is the intent of the top most if condition, i.e.:

k8s.io/groups/service.go

Line 187 in 75c0411

if group.Name != "" && grp.Name != group.Name ||

to make sure that updates do not clear names and descriptions of the google groups?

If that is the intent then, the code above this will also need to be changed:

k8s.io/groups/service.go

Lines 168 to 181 in 75c0411

    
           g := admin.Group{ 
        
           	Email: group.EmailId, 
        
           } 
        
           if group.Name != "" { 
        
           	g.Name = group.Name 
        
           } 
        
           if group.Description != "" { 
        
           	g.Description = group.Description 
        
           } 
        
           g4, err := as.client.InsertGroup(&g) 
        
           if err != nil { 
        
           	return fmt.Errorf("unable to add new group %q: %w", group.EmailId, err) 
        
           } 
        
           log.Printf("> Successfully created group %s\n", g4.Email)

since there is a possibility of groups with empty names and descriptions being created.

MadhavJivrajani · 2021-10-13T06:57:18Z

/test all

MadhavJivrajani · 2021-10-13T09:46:09Z

/test ?

k8s-ci-robot · 2021-10-13T09:46:10Z

@MadhavJivrajani: The following commands are available to trigger required jobs:

/test pull-k8sio-backup
/test pull-k8sio-cip
/test pull-k8sio-file-promo
/test pull-k8sio-groups-test
/test pull-k8sio-terraform-kubernetes-public
/test pull-k8sio-terraform-prow-build
/test pull-k8sio-terraform-prow-build-trusted
/test pull-k8sio-terraform-public-pii
/test pull-k8sio-verify

Use /test all to run the following jobs that were automatically triggered:

pull-k8sio-groups-test
pull-k8sio-verify

In response to this:

/test ?

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.

MadhavJivrajani · 2021-10-14T07:25:48Z

@spiffxp the PR's ready for review, I've added the required tests, PTAL :)

Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>

- Stub out clientErrCheckFunc for testing so that returned errors from fake client can be checked easily. - Add methods to create admin and group services by taking in custom clients and clientErrCheckFunc Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>

Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>

spiffxp

I've got some style / corner-case nits. Overall though I think this is great coverage.

groups/fake/fake_client.go

spiffxp · 2021-10-14T14:10:01Z

groups/fake/fake_client.go

+	// Groups is a mapping from groupKey to *admin.Group
+	Groups map[string]*admin.Group
+	// Members is a mapping from groupKey -> members of that group
+	// Members of a group are a mapping from memberKey -> *admin.Member
+	Members map[string]map[string]*admin.Member


map[string]map[string] feels off. Makes me wonder if we're missing a type that holds both the Group and its Members

groups/fake/fake_client.go

groups/reconcile_test.go

groups/service_test.go

spiffxp · 2021-10-14T15:12:05Z

groups/reconcile_test.go

+						"ReconcileMembers":         "true",
+					},
+					Members:  []string{}, // member removed


Do we have a test case that confirms ReconcileMembers: false behavior for a member? Like proof that attempting to remove a member won't work if this setting is false

spiffxp · 2021-10-14T15:13:48Z

groups/service_test.go

+	"k8s.io/k8s.io/groups/fake"
+)
+
+func augmentAdminServiceFakeClient(fakeClient *fake.FakeAdminServiceClient) *fake.FakeAdminServiceClient {


Ah, here's where the fixtures are hidden. I feel like it would be better to put these closer to the reconcile tests so I don't have to keep bouncing back and forth here. I don't have a suggestion off the top of my head though.

FWIW I also wonder if it'd be better to pass this data in via the constructor, then less code needs to be aware it's dealing with / mutating a fake

How about providing a constructor that returns an augmented fake client?

groups/service_test.go

groups/service.go

Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>

MadhavJivrajani · 2021-10-27T06:20:22Z

groups/reconcile_test.go

+		// desired state is not nescessarily the same as expected state.
+		// We might need the two of them to differ in cases where we want
+		// to test out functionality where our desired state warrants for
+		// a change but in actuality this change should not occur - which
+		// is signified via expected state. shouldConsiderExpectedState is
+		// what differentiates what we should consider when comparing results.
+		// If false - desired is same as expected. This is done because in
+		// most cases these two are same and we can avoid clutter.
+		shouldConsiderExpectedState bool
+		desiredState                []GoogleGroup
+		expectedState               []GoogleGroup


@spiffxp I've added a test for testing out reconciliation for ReconcileMembers setting set to "false". In order to do so, we would need another dimension called expectedState to verify if things worked correctly for such a case. However, adding expectedState to all test cases resulted in a lot of bloat - which is why I went with this approach. Let me know if this is okay or if I should change it.

MadhavJivrajani · 2021-10-27T06:21:04Z

@spiffxp I've addressed most comments, PTAL

spiffxp

I think the test data could be pruned down a bit still but this has been hanging out enough, improvements can be done as followup

/approve
/lgtm

k8s-ci-robot · 2021-11-02T16:28:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MadhavJivrajani, spiffxp

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:

~~groups/OWNERS~~ [spiffxp]

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 requested review from spiffxp and thockin October 4, 2021 08:08

k8s-ci-robot added the sig/k8s-infra Categorizes an issue or PR as relevant to SIG K8s Infra. label Oct 4, 2021

k8s-ci-robot assigned spiffxp Oct 4, 2021

MadhavJivrajani force-pushed the groups-test-coverage branch 4 times, most recently from 6b8a5ce to 94abbfe Compare October 12, 2021 06:29

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 13, 2021

MadhavJivrajani marked this pull request as ready for review October 13, 2021 07:15

MadhavJivrajani force-pushed the groups-test-coverage branch from 3a13fe9 to aa5a6f3 Compare October 13, 2021 07:50

MadhavJivrajani force-pushed the groups-test-coverage branch from aa5a6f3 to 9e96b96 Compare October 14, 2021 07:23

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 14, 2021

MadhavJivrajani changed the title ~~[WIP] Increase test coverage for groups/~~ Increase test coverage for groups/ Oct 14, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 14, 2021

MadhavJivrajani added 5 commits October 14, 2021 13:41

add fake clients for admin and group services

2954c04

Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>

add tests for admin service

d318a3f

Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>

add tests for group service

1a84ae2

Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>

add tests for reconciliation logic

9db32f0

Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>

MadhavJivrajani force-pushed the groups-test-coverage branch from 9e96b96 to 9db32f0 Compare October 14, 2021 08:14

spiffxp suggested changes Oct 14, 2021

View reviewed changes

MadhavJivrajani added 3 commits October 27, 2021 11:46

move errFunc into constructor and rectify equality checks

75cc48d

Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>

move fixtures and augmenting fake client into constructor

da76faa

Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>

add tests for ReconcileMembers setting to be false

115e715

Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>

MadhavJivrajani commented Oct 27, 2021

View reviewed changes

MadhavJivrajani mentioned this pull request Nov 2, 2021

groups: Add MadhavJivrajani to reviewers #3014

Merged

spiffxp approved these changes Nov 2, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 2, 2021

k8s-ci-robot merged commit bdc301b into kubernetes:main Nov 2, 2021

k8s-ci-robot added this to the v1.23 milestone Nov 2, 2021

MadhavJivrajani mentioned this pull request Feb 16, 2022

[Umbrella Issue] Potential Code Improvements kubernetes-sigs/maintainers#12

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase test coverage for groups/ #2873

Increase test coverage for groups/ #2873

MadhavJivrajani commented Oct 4, 2021 •

edited

k8s-ci-robot commented Oct 4, 2021

MadhavJivrajani commented Oct 4, 2021

MadhavJivrajani commented Oct 4, 2021

MadhavJivrajani commented Oct 12, 2021 •

edited

MadhavJivrajani commented Oct 13, 2021

MadhavJivrajani commented Oct 13, 2021

k8s-ci-robot commented Oct 13, 2021

MadhavJivrajani commented Oct 14, 2021 •

edited

spiffxp left a comment

spiffxp Oct 14, 2021

spiffxp Oct 14, 2021

spiffxp Oct 14, 2021

MadhavJivrajani Oct 21, 2021

MadhavJivrajani Oct 27, 2021

MadhavJivrajani commented Oct 27, 2021

spiffxp left a comment

k8s-ci-robot commented Nov 2, 2021

Increase test coverage for groups/ #2873

Increase test coverage for groups/ #2873

Conversation

MadhavJivrajani commented Oct 4, 2021 • edited

k8s-ci-robot commented Oct 4, 2021

MadhavJivrajani commented Oct 4, 2021

MadhavJivrajani commented Oct 4, 2021

MadhavJivrajani commented Oct 12, 2021 • edited

MadhavJivrajani commented Oct 13, 2021

MadhavJivrajani commented Oct 13, 2021

k8s-ci-robot commented Oct 13, 2021

MadhavJivrajani commented Oct 14, 2021 • edited

spiffxp left a comment

Choose a reason for hiding this comment

spiffxp Oct 14, 2021

Choose a reason for hiding this comment

spiffxp Oct 14, 2021

Choose a reason for hiding this comment

spiffxp Oct 14, 2021

Choose a reason for hiding this comment

MadhavJivrajani Oct 21, 2021

Choose a reason for hiding this comment

MadhavJivrajani Oct 27, 2021

Choose a reason for hiding this comment

MadhavJivrajani commented Oct 27, 2021

spiffxp left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 2, 2021

MadhavJivrajani commented Oct 4, 2021 •

edited

MadhavJivrajani commented Oct 12, 2021 •

edited

MadhavJivrajani commented Oct 14, 2021 •

edited