Increased unit test coverage #72
Conversation
|
The subsystem tests are broken... please fix it and mention us again |
isaacdorfman
force-pushed
the
increase-unit-test-coverage
branch
from
0a8678d to
c27b2dd
Compare
provider/cluster_resource.go
Outdated
| @@ -39,6 +47,98 @@ type ClusterResource struct { | |||
| collection *cmv1.ClustersClient | |||
| } | |||
|
|
|||
| type ClusterClient interface { | |||
There was a problem hiding this comment.
I think the new interface and its implementation should be located in another package/file
@bardielle what do you think
There was a problem hiding this comment.
@nirarg agree with you.
I think it's important in order to reuse it later in cluster rosa classic resource
There was a problem hiding this comment.
@isaacdorfman this interface must be align with the upcoming changes in cluster_rosa_classic resource
so if need to add more interface's methods please add them and later implements that with return or whatever is needed in the simple cluster resource
In that way you'll have 3 files
- interface
- implementation for cluster resource
- implementation for cluster rosa classic resource
- cluster resource (?)
| @@ -36,10 +36,17 @@ ldflags:=\ | |||
| -X $(import_path)/build.Commit=$(commit) \ | |||
| $(NULL) | |||
|
|
|||
| .PHONY: gen-mocks | |||
| gen-mocks: | |||
| mockgen -source=provider/cluster_resource.go -destination=provider/cluster_resource_mock.go -package provider | |||
There was a problem hiding this comment.
Instead of adding specific files here in the Makefile, add go:generate mockgen comment near the interface to be mocked and use go:genernate command recursively in the project directory
provider/cluster_resource.go
Outdated
| type ClusterResourceUtils interface { | ||
| populateClusterState(object *cmv1.Cluster, state *ClusterState) | ||
| createClusterObject(ctx context.Context, | ||
| state *ClusterState, diags diag.Diagnostics) (*cmv1.Cluster, error) | ||
| } | ||
|
|
||
| type ClusterResourceUtilsImpl struct { | ||
| clusterClient ClusterClient | ||
| } | ||
|
|
There was a problem hiding this comment.
Also this new Interface and its implementation should be moved to different package/File
| clusterClient := &ClusterClientImpl{InternalClient: r.collection} | ||
| clusterUtils := ClusterResourceUtilsImpl{clusterClient: clusterClient} |
There was a problem hiding this comment.
It should be created only once when calling to ClusterResource constructor and not in each function
provider/cluster_resource_test.go
Outdated
|
|
||
| It("Creates ClusterBuilder with correct field values", func() { | ||
| generateBaseClusterMap := func() map[string]interface{} { |
There was a problem hiding this comment.
Please move all those functions out from the describe
You can use "var" scope
provider/cluster_resource_test.go
Outdated
| @@ -175,4 +239,248 @@ var _ = Describe("Cluster creation", func() { | |||
| Expect(clusterState.AWSSecretAccessKey.Value).To(Equal(awsSecretAccessKey)) | |||
| Expect(clusterState.AWSPrivateLink.Value).To(Equal(privateLink)) | |||
| }) | |||
|
|
|||
| Describe("clusterUtils::create", func() { | |||
There was a problem hiding this comment.
The describes doesn't describe what you would like to test in that scope
provider/cluster_resource_test.go
Outdated
| }) | ||
| }) | ||
|
|
||
| Describe("clusterUtils::update", func() { |
provider/cluster_resource_test.go
Outdated
|
|
||
| Describe("clusterUtils::update", func() { | ||
| It("invokes cluster update on the client", func() { | ||
| ctrl := generateGinkgoController() |
There was a problem hiding this comment.
Use the section Beforeach()
{
ctrl := generateGinkgoController()
clusterCurrentState := generateBaseClusterState()
clusterDesiredState := generateBaseClusterState()
clusterDesiredState.ComputeNodes.Value = clusterDesiredState.ComputeNodes.Value + 1
clusterMap := generateBaseClusterMap()
clusterMap["nodes"].(map[string]interface{})["compute"] = clusterDesiredState.ComputeNodes.Value
}
provider/cluster_resource_test.go
Outdated
| }) | ||
| }) | ||
|
|
||
| Describe("clusterUtils::delete", func() { |
isaacdorfman
force-pushed
the
increase-unit-test-coverage
branch
from
c27b2dd to
36a5775
Compare
No description provided.