Conversation
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| <% end -%> | ||
| <%# |
There was a problem hiding this comment.
I love this description.
templates/puppet/integration_spec.rb
Outdated
| puts 'pre-destroying <%= Google::StringUtils.underscore(obj.name) -%>' | ||
| VCR.use_cassette('pre_destroy_<%= Google::StringUtils.underscore(obj.name) -%>') do | ||
| example = get_example('delete_<%= Google::StringUtils.underscore(obj.name) -%>') | ||
| test = Puppet::Node::Environment.create(:test, mods) |
There was a problem hiding this comment.
Is there any way we can hide lines 110-118 (ish) in a function somewhere?
It would make the tests more readable at a higher-level
|
First glance (I know, I know, you haven't asked for me to appear from the shadows yet): this looks awesome. |
|
Second glance: in my mind, we're verifying that things are actually created/destroyed/edited in GCP by verifying the VCR cassettes by-hand and verifying that the requests look reasonable. Right? |
| confirming that no resources make web requests during 'flush'. | ||
|
|
||
| VCR recordings are called 'cassettes', and they are stored in | ||
| 'spec/cassettes'. If present, they are used - if absent, created. |
There was a problem hiding this comment.
Are we planning on storing the cassettes on github and updating them periodically?
There was a problem hiding this comment.
Planning instead on storing them somewhere auth'd, because they include the raw text of the request we send with our service account private key. But that's not part of this PR just yet - I want to get the tests in first, then integrate them.
nat-henderson
force-pushed
the
puppet-vcr
branch
3 times, most recently
from
e195198 to
fdc0c30
Compare
nat-henderson
force-pushed
the
puppet-vcr
branch
from
fdc0c30 to
ed32747
Compare
|
I am a robot that works on MagicModules PRs! I built this PR into one or more PRs on other repositories, and when those are closed, this PR will also be merged and closed. |
|
Okay, the Terraform PR is probably a mistake - but I'm not sure if it's a mistake with this PR, or some problem with generation. I've asked Dana about that. As for the rest of this, I'm going through and it looks about right so far... |
|
@rambleraptor take a look especially at https://github.com/GoogleCloudPlatform/puppet-google-compute/pull/89/files and let me know what you think. As for validation, right now I'm checking to make sure things look sensible ... they mostly do. :) There are a few places where these tests pass but should fail, but I'm pretty sure those are underlying issues with the way that we report errors in our providers. For instance, anything using the healthcheck function fails to create with a 400, because the healthcheck reference is to a function in a different project. This doesn't crash the test because we don't seem to report it as an error. I'll look into that in parallel. |
| echo "Skipping tests... See issue #236" | ||
| elif [ -z "$EXCLUDE_PATTERN" ]; then | ||
| DISABLE_COVERAGE=true bundle exec parallel_rspec spec/ | ||
| if ls spec/g$PRODUCT* > /dev/null 2&>1; then |
There was a problem hiding this comment.
Can you put a comment somewhere explaining what all of this is doing?
| puts 'confirming <%= Google::StringUtils.underscore(obj.name) -%> destroyed' | ||
| VCR.use_cassette('check_destroy_<%= Google::StringUtils.underscore(obj.name) -%>') do | ||
| validate_no_flush_calls('delete_<%= Google::StringUtils.underscore(obj.name) -%>') | ||
| end |
There was a problem hiding this comment.
Just something to think about:
Imagine I have a Managed Zone with a ResourceRecordSet. I'm testing the ResourceRecordSet in this case.
First, I "destroy" my resource records set to get my environment set up. I don't really care about this network call. If it actually destroys an RRS or doesn't, I don't care. We're testing that part later on. This could make the cassettes weird, as the cassettes would disagree if my environment happened to have the RRS already created.
The second part is the side effects of the "pre-destroy" script. In order to destroy a RRS, I have to verify that my Managed Zone exists. That means, after I call the destroy script, I have no RRS, but I do have a Managed Zone. Even after this entire test suite is run, my Managed Zone will still be lingering.
Here's the pattern I use for Ansible:
- Build all of the dependent resources (blindly, I don't care what happens because we're not testing those resources)
- Build the resource in question - test if it exists
- Build the resource again - test if nothing changed
- Destroy the resource - test if no longer exists
- Destroy the resource again - test that nothing changed
- Destroy all of the dependent resources (blindly, I don't care what happens because we're not testing those resources)
There was a problem hiding this comment.
Ooh, interesting. Several parts of this I have a response to, the rest I'm still chewing on.
This could make the cassettes weird, as the cassettes would disagree if my environment happened to have the RRS already created.
This is okay - if the RRS exists beforehand, the cassette will show a destroy ... but since the cassette also includes the check to see if the RRS exists, it won't ever be inconsistent until the next time we re-record the cassettes.
The second part is the side effects of the "pre-destroy" script. In order to destroy a RRS, I have to verify that my Managed Zone exists. That means, after I call the destroy script, I have no RRS, but I do have a Managed Zone. Even after this entire test suite is run, my Managed Zone will still be lingering.
This isn't great! We have some foo.pp and delete_foo.pp pairs that, if run in sequence, won't produce an empty environment? I think we should probably fix that!
I think your ansible pattern is a good one, but I think that in Puppet I would have a rough time figuring out which resources are the dependents and which are the resource under test just from looking at the manifest as a string ... and by the time I've got it parsed into a RAL, it's too late to pull out pieces of it, I think. Do you think that would work? I could look into it.
What do you advise?
There was a problem hiding this comment.
On the first part, that sounds fine. I'm a little unclear how we're checking the cassettes to make sure they work properly (are we checking by hand? when will the tests "crash" because the network calls differ? what about headers, access tokens, and the like?), but I'm assuming that'll all become more clear in due time.
On the second part, I've got a mess of words.
Two parts to this:
- How do we run the dependencies? (you probably already know the answer to this)
I think we can do this using good old-fashioned before/after test calls on RSpec.
In the case of RRS - before we run the tests, we run the create-managed-zone example. After we run all of the tests, we run the delete-managed-zone example to clean up. Rspec has before/after test handlers that will make the tests look nice + handle running things before/after for us (you've probably seen/used those before).
This is assuming that the create/delete managed-zone examples use the same name as the create/delete resource-record-set.
- How do we figure out the dependencies?
Now, as for coming up with the dependency patterns, we shouldn't let Puppet figure that out. Magic Modules can pretty easily figure that out though. If you look at the autogenerated Chef tests, you'll see that each autogenerated test is placed the proper order. That's because Chef doesn't build out a graph like Puppet does. If you put the Chef blocks in the wrong order, things just crash. I built out a dependency graph that looks at all of the ResourceRefs and figures out what should be run in what order.
We could use that idea and just output tests that explicitly run tasks in a certain order. Puppet wouldn't have to figure out dependencies or anything like that - it's just running tasks in a prescribed order.
There was a problem hiding this comment.
Interesting. That's very handy! I bet that will work great! What a fun idea. I'll poke at it next week - I need tomorrow for some other work.
There was a problem hiding this comment.
I've thought about this some more, and I'm not inclined to implement it the way you suggested at first glance. Let me see if we're on the same page about this.
Problem with the current implementation:
- dangling resources are possible, or even likely, or even literally-impossible-to-avoid in some resources.
- every test also implicitly tests its dependencies - failures won't be isolated, making root-causing challenging.
But I see some issues with your methodology as well:
- all examples are tied together, meaning that a change to improve testing of
networkwill require 10-50 changes throughout the examples directory. - all examples need to use the same dependencies, which means that we'll have a real hard time with, e.g. VPN Tunnel testing.
- it's not possible to test autorequires - we're forcing create-before-create and destroy-after-destroy behavior that we actually want to confirm works correctly.
- lamely: dependency graph behavior feels complicated and I'm inclined to not add complicated things, because debugging tests is already a pain and I don't want to make it more confusing in there.
So here's my theory. I could rewrite the entire set of examples, splitting them into dependencies and unit-under-test - this is almost done already, honestly, with the 'filename != readme' check that's in most of 'em. Then we could know which tests failed in the before bit and which tests failed in the unit-under-test bit. Then we get rid of the dependency on a delete_foo manifest and just swap the present to absent bit in the unit-under-test, then swap the present to absent bits in the dependencies. 4-step process, most of the perks, few of the flaws.
Downside: that sounds hard to do.
Question: is it worth doing?
There was a problem hiding this comment.
Totally understand the issues.
I don't entirely follow your solution
|
I am (still) a robot that works on MagicModules PRs! I just wanted to let you know that your changes (as of commit 7815e25) have been included in your existing downstream PRs. |
5 participants
This generates VCR tests that can test the interaction of GCP and Puppet. The comment in
templates/puppet/integration_spec.rbsums it up - I'd start there.Still to do: integrate these tests into our CI system, including ensuring that the credentials are present in the path where they need to be, and that the project is set up. Also, we'll need to build or find a way to manage the cassettes. There's work to be done, but as it stands now, the tests ought to be useful. :)
[all]
[terraform]
[puppet]
Add VCR-based integration / unit tests.
[puppet-bigquery]
[puppet-compute]
[puppet-container]
[puppet-dns]
[puppet-logging]
[puppet-pubsub]
[puppet-resourcemanager]
[puppet-sql]
[puppet-storage]
[chef]
[chef-compute]
[chef-container]
[chef-dns]
[chef-logging]
[chef-spanner]
[chef-sql]
[chef-storage]
[ansible]