Add acceptance tests for schema dependency auto-detection #2169
Conversation
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
changed the title
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
requested review from
pietern,
andrewnester and
denik
as code owners
| { | ||
| "pipeline1": { | ||
| "catalog": "catalog1", | ||
| "schema": "${resources.schemas.schema1.name}" |
There was a problem hiding this comment.
Is it expected that this is not resolved to the name? We have the name, right?
There was a problem hiding this comment.
Is it expected that this is not resolved to the name? We have the name, right?
The terraform layer in the stack resolves any values with the resources prefix. bundle/deploy/terraform/interpolate.go is the translation layer between these references and syntax that terraform understands.
There was a problem hiding this comment.
Does it work for 'name' as well? I thought it's for looking up ids.
In any case, could you add this context as a comment on script?
There was a problem hiding this comment.
Should we also add 'bundle summary' to see the resolution in action?
There was a problem hiding this comment.
Does it work for 'name' as well? I thought it's for looking up ids
Yeah, terraform resolves any fields that are keys in it.
Should we also add 'bundle summary' to see the resolution in action?
Bundle summary does not resolve these references. It's resolved at runtime when terraform apply happens during bundle deploy.
"volumes": {
"abc": {
"catalog_name": "main",
"modified_status": "created",
"name": "abc",
"schema_name": "${resources.schemas.xyz.name}",
"volume_type": "MANAGED"
}
}
},
| @@ -0,0 +1 @@ | |||
| $CLI bundle validate -o json | jq .resources.pipelines | jq 'map_values(del(.deployment, .permissions))' > out.json | |||
There was a problem hiding this comment.
Could you add a small comment there what do you expect to see in the output.
Also it should be clear -- is the output correct or is there something we want to fix?
There was a problem hiding this comment.
There's nothing to fix. I'd assume that the tests here are for correct behaviour by default. Should we explicitly call that out in all acceptance tests?
There was a problem hiding this comment.
I see. Normally, it's not needed, but ${resources.*} behaviour is a bit surprising, so worth calling out.
There was a problem hiding this comment.
Added a comment for the expectation with the test itself.
| { | ||
| "pipeline1": { | ||
| "catalog": "catalog1", | ||
| "schema": "${resources.schemas.schema1.name}" |
There was a problem hiding this comment.
Does it work for 'name' as well? I thought it's for looking up ids.
In any case, could you add this context as a comment on script?
| @@ -0,0 +1,43 @@ | |||
| bundle: | |||
There was a problem hiding this comment.
cli/bundle/config/mutator/capture_schema_dependency_test.go
Line 1 in 2e70558
package mutator
Do we need the unit tests? Is there something they cover that this acceptance tests won't?
There was a problem hiding this comment.
Do we need the unit tests? Is there something they cover that these acceptance tests won't?
No, mostly. They're local to the mutator itself, which implements the functionality, and that's why I did not remove them. Unit tests next to the code are nice to have and can live independently of the acceptance tests.
There's some additional coverage for what happens when the resources pointers are nil, but nothing that absolutely needs to be covered, IMO.
| { | ||
| "pipeline1": { | ||
| "catalog": "catalog1", | ||
| "schema": "${resources.schemas.schema1.name}" |
There was a problem hiding this comment.
Should we also add 'bundle summary' to see the resolution in action?
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
| b := &bundle.Bundle{ | ||
| Config: config.Root{ | ||
| Resources: config.Resources{ | ||
| Schemas: map[string]*resources.Schema{ | ||
| "schema1": { |
There was a problem hiding this comment.
I'm on the fence regarding whether we should remove this unit test here. On one hand, it serves as documentation regarding what the mutator does. On the other hand, it duplicates the acceptance test content.
More generally, what do you guys think about maintaining unit tests that are copies of acceptance tests that we have? cc: @pietern @andrewnester @denik
There was a problem hiding this comment.
I think those should be removed. If it's not adding coverage, it's just maintenance without the upside.
Changes
This PR converts the unit tests for capturing schema dependency to acceptance tests. Acceptance tests are nicer because they test the config transformation end to end.