tests: create more tests for monitoring dashboard #1806
Conversation
justinsb
commented
May 15, 2024
justinsb
commented
- tests: create more tests for monitoring dashboard
- mockgcp: more fidelity for project creation
- tests: pass a sensible default folder id in hack/record-gcp
justinsb
force-pushed
the
monitoring_dashboard_more_tests
branch
from
2f58ae8
to
a0bf4b6
Compare
There was a problem hiding this comment.
did we mean to regenerate this?
There was a problem hiding this comment.
So this file should have moved, we don't have a consistent testfixture structure, so when we add our second test we have to move the first one into a subdirectory. What happened here is that git detected some of the deletions as renames - I think the final state is correct.
(FYI, git renames are a client-side construct ... and so are diffs)
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return lroV3ToV1(lro) | ||
| // Note: Unclear if we should wait for the operation, I _think_ no. |
There was a problem hiding this comment.
why do we not need to wait for the op to finish?
There was a problem hiding this comment.
So V1 doesn't return an LRO. I think we get away with it here because V3 DeleteProject is actually synchronous. I'll expand the comment.
There was a problem hiding this comment.
Actually, I decided it was easier just to wait for the operation. Then we don't have to worry about the implementation details of V3 DeleteProject
hack/record-gcp
Outdated
| @@ -9,6 +9,10 @@ rm -rf $(pwd)/artifactz/realgcp | |||
|
|
|||
| RUN_TESTS=TestAllInSeries/$1 | |||
|
|
|||
| DEFAULT_PROJECT=$(gcloud config get project) | |||
There was a problem hiding this comment.
just a reminder that this will fail in GH actions. It would be helpful if we "err out" or log out if any of these are empty or can use my trick to ?= and accept whatever we set in GH env.
There was a problem hiding this comment.
Yes, fair, though this script is meant for use locally.
I'll change it to default the project if not set!
Including around refs
It's not perfect (v3 vs v1 is tricky), but it's much closer.
This allows us to record test output even when we must create a project.
justinsb
force-pushed
the
monitoring_dashboard_more_tests
branch
from
a0bf4b6
to
defba09
Compare
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return lroV3ToV1(lro) | ||
| // Note: Unclear if we should wait for the operation, I _think_ no. |
There was a problem hiding this comment.
Actually, I decided it was easier just to wait for the operation. Then we don't have to worry about the implementation details of V3 DeleteProject
| filter: metric.type="agent.googleapis.com/nginx/connections/accepted_count" | ||
| resourceNames: | ||
| - external: "projects/other${uniqueId}" | ||
| # TODO: Switch to a ref |
There was a problem hiding this comment.
This is a bummer, although it opens a door.
Looks like logsPanel.resourceNames with a reference to a Project is broken in TF/DCL.
The good news is that because it's broken, I think we have more wiggle room in terms of whether kind should be required etc.
Sadly, the test is not very interesting because of this bug, but we'll fix that as part of going direct!
| User-Agent: google-api-go-client/0.5 Terraform/ (+https://www.terraform.io) Terraform-Plugin-SDK/2.10.1 terraform-provider-google-beta/kcc/controller-manager | ||
| X-Goog-Api-Client: gl-go/1.22.3 gdcl/0.177.0 | ||
|
|
||
| 200 OK |
There was a problem hiding this comment.
It's a pity that many server uses 200 rather than 204 on empty response.
| User-Agent: google-api-go-client/0.5 Terraform/ (+https://www.terraform.io) Terraform-Plugin-SDK/2.10.1 terraform-provider-google-beta/kcc/controller-manager | ||
| X-Goog-Api-Client: gl-go/1.22.3 gdcl/0.177.0 | ||
|
|
||
| 403 Forbidden |
There was a problem hiding this comment.
oh, didn't realize this returns 403 rather than 404.
|
|
||
| // Wait for the operation to complete. | ||
| // If the operation is not done after timeout, an error is returned. | ||
| func (o *Operations) Wait(ctx context.Context, opName string, timeout time.Duration) (*longrunningpb.Operation, error) { |
There was a problem hiding this comment.
Do you mind adding a little bit more comment to suggest when and how to use the wait? Or is this to make the mock LRO be more like a real LRO? I assume mock tests should be done pretty fast.
|
/lgtm No blocker and the PR looks great! feel free to resolve comments in follow up PRs. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yuwenma The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
google-oss-prow
bot
merged commit b17e40c
into
GoogleCloudPlatform:master
