-
Notifications
You must be signed in to change notification settings - Fork 193
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
Controller tests - mocked resource manager #226
Conversation
Hey @szoio sorry for the slow review here, been out of town. I will have a review submitted by EOD Monday! |
chore: bump deps and minor cleanup
Thanks @frodopwns. I've updated the PR description to summarise the changes. |
@szoio could you add me as a collaborator on your fork? Looks like my access went away. |
Added. I recreated my fork but forgot to add you back. |
Everything is running fine. Takes about 4.5 minutes to run the full test suite. Still want to dig in a little more to see what kinds of issues it can catch. |
Was able to catch a failure by commenting out the r.Update calls that set provisioning to true or false...this is good have you tested any other scenarios that might result in failures being caught? |
Do you think we should update the tests to also check that the resource exits provisioning state and enters provisioned state? |
I think we should verify any of the states that matter to the controller. If it doesn't affect the controller in any way, then we shouldn't test it, because we are neither trying to test the resource manger nor the sdk. I think there is scope to improve the assertions we are making and make them more comprehensive in a lot of cases, whether we are using the mocks or the real resource manager. |
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.
👍 this looks like a good start, we can talk about abstracting this a bit more once we settle on a abstraction layer for the resource manager clients
might be a good idea to make sure the pipeline runs both test scenarios before merging this |
Thanks @frodopwns for approving. |
What this PR does / why we need it:
TEST_CONTROLLER_WITH_MOCKS
(true or false, default value true) that determines if the controller tests are run with mocked resource managers.make test
to run with mocks, andmake test-existing
to run direct with Azure.When running with mocks, controller tests now take 5-6 seconds.
This PR refs #228
Special notes for your reviewer:
The mocked resource managers are quite crude and rudimentary as simple arrays of resources - would be nice to to this with generics if they existed!
If applicable: