-
Notifications
You must be signed in to change notification settings - Fork 94
Conversation
/ok-to-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really cool to see this taking shape!
@@ -0,0 +1,515 @@ | |||
/* | |||
Copyright 2017 The Kubernetes Authors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: 2018
} | ||
} | ||
|
||
func TestMarshalOptions(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the intention to migrate this to a shared test later? MarshalOptions
isn't in this package.
"k8s.io/cluster-registry/pkg/crinit/util" | ||
) | ||
|
||
func TestValidateOptions(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these tests specific to the aggregated command? It seems like it would make sense to add only tests for the aggregated part here, since the standalone test is already testing these things and you're intending to refactor it in a follow-up.
} | ||
} | ||
|
||
func TestCreateNamespace(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: this is already being tested in the standalone
tests. It feels to me like a cleaner path towards the final goal is to add a few tests for the aggregated
-specific functionality, refactor the standlone
tests into common
, and then flesh out the tests for aggregated
and standalone
specific features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I wanted to get something simple up for a start to get some ideas. I'm going to apply the [WIP] tag and work on making RBAC and APIService tests for aggregated first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes total sense to me. Looking forward to seeing the tests that you come up with!
ce36852
to
4facace
Compare
@onyiny-ang Is this still WIP, or are you ready for a review? |
@perotinus it's just about ready. I'm refactoring what I have to reduce code duplication and then will reset the commits to separate the manual changes from the dep/test changes as noted in the docs. |
@perotinus @font This is ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Great to see these tests coming together!
func TestCreateRBACObjects(t *testing.T) { | ||
testCases := []struct { | ||
desc string | ||
buffer *bytes.Buffer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need this in the test setup struct: it's the same in each test, so it can probably just live in the test.
IMO the things that should be here are the things that vary between tests and/or are relevant/interesting for someone trying to understand the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. That makes sense to me.
if err != nil { | ||
t.Error("Failed to create an RBAC Object") | ||
} | ||
chkrbac, _ := tc.client.CoreV1().ServiceAccounts(tc.opts.ClusterRegistryNamespace).Get(rbacObj.Name, metav1.GetOptions{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test will need to be extended in a later PR to validate the contents of the returned object, and the other objects being created as well: this method creates several resources, and we want to make sure that the created objects match our expectations as to what they will contain.
if err != nil { | ||
t.Error("Failed to create an RBAC Object") | ||
} | ||
chkrbac, _ := tc.client.CoreV1().ServiceAccounts(tc.opts.ClusterRegistryNamespace).Get(rbacObj.Name, metav1.GetOptions{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createdRBACObj, _ :=
...
?
if err != nil { | ||
t.Error("Error in pre-test generating credentials: %v", err) | ||
} | ||
apisvcObj, err := createAPIService(tc.buffer, tc.apisvcClient, tc.opts, util.GetCAKeyPair(credentials).Cert) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might make this test a bit more robust if you create these dependencies in the test code, rather than relying on crinit
's code to create the service and the credentials.
} | ||
apiService, _ := tc.apisvcClient.ApiregistrationV1beta1().APIServices().Get(apisvcObj.Name, metav1.GetOptions{}) | ||
if tc.svcExpected { | ||
if apiService == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be valuable to test for the contents of the object as well. We expect that they should match what's being provided in the inputs.
/test pull-cluster-registry-verify-bazel |
83bd457
to
bfb46c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates! A few more comments, but I think this is about ready to go in.
t.Errorf("Error in creating API service") | ||
} else { | ||
if apiService.ObjectMeta.Name != apiSvcName { | ||
t.Errorf("Unexpected API Service Created. Expected %v, got %v", apiSvcName, apiService.ObjectMeta.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Unexpected APIService created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure which nit you are indicating. Is it that APIService should have no space? Or that I should remove the following sentence (Expected. . .)? Or that the subsequent error messages have left out Service? I have assumed the latter but am not sure if this what you meant. In any case, that needed to be corrected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was that the following error messages left out Service
. Apologies, I should have made the comment on one of those.
for _, tc := range testCases { | ||
t.Run(tc.desc, func(t *testing.T) { | ||
buffer := &bytes.Buffer{} | ||
//ips := []string{"1.2.3.4"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove this commented-out line?
@@ -15,7 +15,12 @@ | |||
|
|||
[[projects]] | |||
name = "github.com/Azure/go-autorest" | |||
packages = ["autorest","autorest/adal","autorest/azure","autorest/date"] | |||
packages = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What version of dep
are you using? I'm not sure why your packages
items are split by newlines. It's not a big deal, but it makes this diff a little harder to parse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that. I was using the devel version but should be using the right version now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it still formats the Gopkg.lock differently. . .
dep version
dep:
version : v0.3.2
build date : 2017-10-19
git hash : 8ddfc8a
go version : go1.9
go compiler : gc
platform : linux/amd64
Please let me know if anything looks suspicious
objExpected bool | ||
}{ | ||
{ | ||
desc: "should create RBAC options", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/options/object
svcExpected: true, | ||
}, | ||
{ | ||
desc: "should create an API Service", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"should not create an API Service in dry-run mode"
or something similar, perhaps like you have in the RBAC test.
/lgtm Thanks for all your work on this! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: onyiny-ang, perotinus The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Adds crinit aggregated unit tests similar to standalone_test.go. Follow up PR will attempt to refactor for common unit test for standalone and aggregated.
Addresses issue: #114
/sig multicluster
/cc @perotinus @font