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.

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

Add tests to ResourceGroup_Controller #287

Merged
merged 11 commits into from
Oct 15, 2019

Conversation

clmccart
Copy link
Contributor

@clmccart clmccart commented Oct 4, 2019

What this PR does / why we need it:
This PR adds additional tests to the resourcegroup_controller_test.go file.
This is necessary because, while using TDD to recreate the resourcegroup operator as an upskilling exercise, I discovered that you could get all of the current resource group controller tests to pass without ever creating or deleting anything in kubernetes or azure.
This PR adds in tests that prove that the resource group controller actually creates and deletes resource group instances.

Special notes for your reviewer:
To test this:

  • comment out lines 97-98, 108-130 in controllers/resourcegroup_controller.go
  • run "make test" and observe that the tests in controllers/resourcegroup_controller_test.go still all pass
  • now checkout the version of controllers/resourcegroup_controller_test.go contianed in this PR and see that the tests fail as expected with the code commented out.
  • uncomment the code to see the tests pass

If applicable:

  • this PR contains documentation
  • [ X] this PR contains tests

Copy link
Contributor

@szoio szoio left a comment

Choose a reason for hiding this comment

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

Looks good to me. Happy to have additional and better assertions.

controllers/resourcegroup_controller_test.go Show resolved Hide resolved
controllers/resourcegroup_controller_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@frodopwns frodopwns left a comment

Choose a reason for hiding this comment

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

go ahead and merge when the tests finish

@szoio szoio self-requested a review October 10, 2019 23:47
@frodopwns
Copy link
Contributor

@clmccart looks like the most recent failure is complaining about https://github.com/clmccart/azure-service-operator/blob/patch/rg_controller_tests/controllers/resourcegroup_controller_test.go#L40

which referts to azurev1 instead of azurev1alpha1

@frodopwns
Copy link
Contributor

Looks good, thanks @clmccart !

@clmccart clmccart merged commit 897a888 into Azure:master Oct 15, 2019
@clmccart clmccart deleted the patch/rg_controller_tests branch October 15, 2019 02:09
Porges pushed a commit that referenced this pull request May 11, 2021
… types (#287)

* Foo.name -> Foo.fooName to avoid confusion with FakeResource.name

I was getting very confused by what I thought was FakeResource.name
being optional, when I hadn't clicked that both FakeResource and Foo
have .name and Foo.name is optional.

* Deepcopy maps, arrays and optional types

This required some special handling in the case of enums. These are
handled by adding a flag to the params that suppresses conversion for
TypeNames when the type names are the same, since the only thing that
will be true for is enums - every other type will have an arm variant
generated..

* Removed unnecessary if statement from map copying in ConvertToArm

Since we create the target map before the check and iterating over a
nil map does the right thing, the `if sourceMap != nil` check isn't needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants