test: refactor armResourceGroupValidationPolicy and add unit test cases#5354
test: refactor armResourceGroupValidationPolicy and add unit test cases#5354mvacula02 wants to merge 1 commit into
Conversation
refactor Azure client policy and add 2 negative unit tests https://redhat.atlassian.net/browse/ARO-26910
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mvacula02 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR refactors armResourceGroupValidationPolicy to use an injectable resource group client, enabling better unit testing and decoupling policy behavior from Azure SDK client construction.
Changes:
- Introduced a
resourceGroupClientinterface anddefaultResourceGroupClientimplementation. - Updated
armResourceGroupValidationPolicyto call the injected client instead of constructing an SDK client inline. - Expanded tests to cover ResourceGroupNotFound-to-CloudError translation and propagation of other errors.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/util/framework/per_invocation_framework.go | Wires the policy to a default resource group client using the invocation credentials. |
| test/util/framework/azure_client_policies.go | Adds injectable RG client abstraction and refactors policy to use it. |
| test/util/framework/azure_client_policies_test.go | Adds a fake RG client and new test cases for 404 CloudError and error propagation. |
| resp, err := pipeline.Do(req) | ||
| require.NoError(t, err) | ||
| assert.False(t, called, "transport should not have been called") | ||
| assert.Equal(t, http.StatusNotFound, resp.StatusCode) | ||
| assert.Equal(t, "application/json", resp.Header.Get("Content-Type")) | ||
|
|
||
| var cloudErr arm.CloudError | ||
| err = json.NewDecoder(resp.Body).Decode(&cloudErr) |
There was a problem hiding this comment.
This is a good point btw. Type of resp.Body is io.ReadCloser, so we are expected to call Close() on it when we finis reading. Adding defer resp.Body.Close() is a good idea.
mbukatov
left a comment
mbukatov
left a comment
There was a problem hiding this comment.
I suggest addressing one copilot comment, but otherwise it looks good.
| } | ||
|
|
||
| _, err = client.Get(req.Raw().Context(), rgName, nil) | ||
| err = p.rgClient.Get(req.Raw().Context(), subID, rgName) |
There was a problem hiding this comment.
Nice approach. This keeps the client initialization per call.
| resp, err := pipeline.Do(req) | ||
| require.NoError(t, err) | ||
| assert.False(t, called, "transport should not have been called") | ||
| assert.Equal(t, http.StatusNotFound, resp.StatusCode) | ||
| assert.Equal(t, "application/json", resp.Header.Get("Content-Type")) | ||
|
|
||
| var cloudErr arm.CloudError | ||
| err = json.NewDecoder(resp.Body).Decode(&cloudErr) |
There was a problem hiding this comment.
This is a good point btw. Type of resp.Body is io.ReadCloser, so we are expected to call Close() on it when we finis reading. Adding defer resp.Body.Close() is a good idea.
ARO-26910
What
Refactor armResourceGroupValidationPolicy to accept an external ARM resource group client.
Add test scenarios:
RG doesn't exist → policy returns a synthetic 404 CloudError response with ResourceGroupNotFound
Other than 404 error occurs → error is propagated as-is
Why
Be able to mock RG client and test negative scenarios.
Testing
The PR adds 2 unit tests
PR Checklist