[Epic] Increase SQL Test Coverage #374
Comments
|
Discussing with QA on how we can improve this.
|
Wait, sorry, can you say more about this idea? I'm not sure how we'd plug in SLT to Terraform.
I don't know that we're seeing a lot of bugs resulting from backwards incompatible changes in Materialize breaking the Terraform provider? Most of the bugs seem to be a result of folks using the Terraform provider in ways that we don't test for. E.g., combinations of options that we've not tested, or, more often, issuing plan updates in a way that updates or deletes existing resources. I'm not sure there's much of a solution here besides building out a massive suite of acceptance tests in this repository that covers more ground. |
|
For sqllogictest I mostly meant we can use that to ensure we cover all the SQL use cases within the Terraform provider as unit tests (in the |
|
Sorry, still not sure I follow! sqllogictest is largely about testing the logic of The Terraform provider doesn't care about the Materialize-side behavior of what it creates at all, as long as it creates it successfully! E.g., for a Kafka sink, the test suite here doesn't need to verify that the sink writes anything in particular to Kafka—just needs to verify that the Terraform So, I'm not sure that you need to go spelunking through the sqllogictest/testdrive suites to determine what to test here! I'd advocate for a coverage-focused approach. As a north star, the ideal would be that for every resource in the Terraform provider, we have coverage of setting and changing every property to a wide variety of values, both valid and invalid. This is no small feat, and of course you can't test every possible combination of properties, so there's an art to structuring the tests to cover the important cases, without massive duplication of tests or exploding the runtime of the test suite. One thing we should explore here is code coverage: https://go.dev/blog/integration-test-coverage. It's proven valuable in the database repo. Ideally we'd have 100% code coverage of the Terraform provider. That certainly doesn't guarantee you've tested all the important cases (e.g., adding and removing grants over multiple By the way, I generally find that unit tests don't provide much value. Not zero value, but not much! They're easy to write and they feel protective, but in my experience they don't tend to surface many bugs. You mostly just lock in the bugs that you have. E.g., the provider is generating SQL that Materialize won't accept, and the test just verifies that the provider produces that incorrect SQL. The bugs tend to be in the integration between Materialize and the provider, and the only way to find those bugs is to test the integration itself. I'd advocate for plowing as much possible bandwidth into the acceptance tests rather than into unit tests—especially acceptance tests that simulate more real world use cases, like evolving sources and grants over multiple The places where you need to rely on unit testing are hopefully relatively few and far between. You'll need it for testing Kafka sources that use PrivateLink connections, as there's no way to write an acceptance test for that without hitting real AWS, and that's probably not worth the trouble. But we can help you pull in some of the mzcompose workflows from the database repo that stand up Kafka and an SSH bastion, for example, and then the Terraform provider can have real acceptance tests that use SSH bastions (if you don't have them already—I didn't check!). |
|
Resource Test Coverage
|
The Terraform provider has to maintain its own client to generate and execute SQL DDL. As part of maintaining the provider we need to ensure the that client has as close to full coverage as possible. Within the provider there are several places where we can test SQL generation:
In trying to expand test coverage. The easiest additional tests would be in
materializeandresources. Here we can add any number of permutations. Currently we try and write tests based on our docs but if there is a place in testing with a more exhaustive list, we can use that. And while it is slightly redundant we will need to include tests for bothmaterializeandresourcesto ensure the correct attributes are passed and parsed by the builders.The integration tests give us the most confidence but require the most additional work to setup. Especially in testing all the various configurations for permutations that depend on additional infrastructure (such as private link).
The text was updated successfully, but these errors were encountered: