-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Functional generator of unit tests. #429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
be55211
8af5373
480e9d3
9ab06bc
863e7e7
ed32747
d3b895c
f60ad22
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| require 'puppetlabs_spec_helper/rake_tasks' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| --- | ||
| <% if false # the license inside this if block pertains to this file -%> | ||
| # Copyright 2017 Google Inc. | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| <% end -%> | ||
| <%= compile 'templates/license.erb' -%> | ||
|
|
||
| <%= lines(autogen_notice :ruby) -%> | ||
|
|
||
| fixtures: | ||
| forge_modules: | ||
| gauth: google/gauth | ||
| symlinks: | ||
| <%= @api.prefix -%>: "#{source_dir}" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,130 @@ | ||
| <% if false # the license inside this if block pertains to this file -%> | ||
| # Copyright 2017 Google Inc. | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| <% end -%> | ||
| <%# | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I love this description. |
||
| This file is complicated, and deserves some documentation here, so that | ||
| you, brave adventurer, can understand what's happened. This file's | ||
| goal is to run the examples required by puppet compilation. Essentially, | ||
| you can think of it as a shell script that runs 'puppet apply examples/foo.pp', | ||
| then 'puppet apply examples/delete_foo.pp', and checks for errors. | ||
|
|
||
| It has to be more complicated than that. :) | ||
| "Fiddling with puppet internal APIs is a lonely and painful path to take." | ||
| - David Schmitt, Puppet SWE, September 2018 | ||
|
|
||
| First, let's talk about the reason that it *isn't* a shell script. | ||
| We need unit tests that really, really validate our puppet modules - we | ||
| can't have them breaking on us, because the community isn't active enough | ||
| that we can trust that we'll find out through regular channels. We | ||
| absolutely must have tests that we can count on to tell us if the module | ||
| is working as expected. And those tests need to run on every PR, because | ||
| these modules are complicated enough that they can break in subtle ways. | ||
| Consequently, we need to be able to run tests that validate use against the | ||
| real GCP APIs, but without spending incredible amounts of time (or money) | ||
| or actually creating hordes of resources. That means a VCR test. | ||
|
|
||
| A VCR test is a kind of test that runs once, making real HTTP requests along | ||
| the way. It records the requests and responses. On later runs, it mocks | ||
| those same HTTP requests with those same responses, making your tests | ||
| repeatable (desirable), fast (desirable), and in our case, also free! | ||
|
|
||
| You can't write a test like that using the puppet command line tool, so | ||
| instead we plumb into the guts of puppet. Our main hook is a function called | ||
| apply_compiled_manifest (and its sibling, apply_with_error_check). | ||
|
|
||
| These functions are not really very similar to `puppet apply`. Instead of | ||
| taking a filename, they take the entire manifest as a string. This causes | ||
| subtle problems deep within Puppet - Puppet depends on the filename of the | ||
| manifest in a handful of ways, not least of which is to find out what | ||
| module it is part of (to automagically load dependencies and functions). | ||
| Since that isn't possible, we tack on the Puppet override of our own | ||
| environment and loader. Since these functions don't call out to Facter | ||
| the way that `puppet apply` does, we need to substitute in our own project | ||
| name and credential path (which all our examples use). Hence the | ||
| get_example() function - it does more than just read the file. | ||
|
|
||
| You can see all this stuff in spec_helper.rb.erb. | ||
|
|
||
| There's also the issue of the begin/rescue pattern you'll see here. It's | ||
| ugly! It's also necessary. compile_to_ral() calls out to our mocked up | ||
| loader, but fails to successfully compile the manifest due to some | ||
| initialization which is mandatory, but isn't possible to complete from | ||
| outside puppet. HOWEVER! We're in luck! In calling some cleanup code, | ||
| in reporting on the failure, compile_to_ral actually *performs* the | ||
| initialization we need. So, if we try it twice, it works the second | ||
| time. Lucky. | ||
|
|
||
| So! We *can* successfully apply a manifest from | ||
| under webmock, as long as we: | ||
| - try twice, eating the first failure. | ||
| - don't need to use any functions that are loaded from modules | ||
| - ensure all our dependent modules have their 'lib' dir in $LOAD_PATH | ||
| - do all our own variable substitution | ||
| - rewrite any necessary functions in puppet and stick them in | ||
| the manifest. | ||
| - import all the magic from puppetlabs_spec_helper/module_spec_helper, | ||
| which I have done without fully understanding it. The comments in | ||
| the code suggest that the authors also do not fully understand it. | ||
|
|
||
| Hopefully that explains the complexity here in a way that will continue | ||
| to be useful as the tests develop further. | ||
|
|
||
| Let me also explain the order in which these commands are run. First, | ||
| there is a pre-destroy - in the event that an older test failed to clean | ||
| up after itself, the next run should take care of it. Second, we create | ||
| the resources in the manifest. Third, we run the create again, mocking | ||
| out any requests during 'flush' and making them crash the test - this is | ||
| to check idempotency of the manifests. Fourth, we delete the resources | ||
| in the manifest, and fifth, we check the idempotency of that delete by | ||
| 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we planning on storing the cassettes on github and updating them periodically?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| #-%> | ||
| <%= compile 'templates/license.erb' -%> | ||
|
|
||
| <%= lines(autogen_notice :puppet) -%> | ||
| require 'spec_helper' | ||
| require 'vcr' | ||
|
|
||
| VCR.configure do |c| | ||
| c.cassette_library_dir = 'spec/cassettes' | ||
| c.hook_into :webmock | ||
| c.configure_rspec_metadata! | ||
| end | ||
|
|
||
| describe '<%= Google::StringUtils.underscore(obj.name) -%>.create', vcr: true do | ||
| it 'creates and destroys non-existent <%= Google::StringUtils.underscore(obj.name) -%>' do | ||
| puts 'pre-destroying <%= Google::StringUtils.underscore(obj.name) -%>' | ||
| VCR.use_cassette('pre_destroy_<%= Google::StringUtils.underscore(obj.name) -%>') do | ||
| run_example('delete_<%= Google::StringUtils.underscore(obj.name) -%>') | ||
| end | ||
| puts 'creating <%= Google::StringUtils.underscore(obj.name) -%>' | ||
| VCR.use_cassette('create_<%= Google::StringUtils.underscore(obj.name) -%>') do | ||
| run_example('<%= Google::StringUtils.underscore(obj.name) -%>') | ||
| end | ||
| puts 'checking that <%= Google::StringUtils.underscore(obj.name) -%> is created' | ||
| VCR.use_cassette('check_<%= Google::StringUtils.underscore(obj.name) -%>') do | ||
| validate_no_flush_calls('<%= Google::StringUtils.underscore(obj.name) -%>') | ||
| end | ||
| puts 'destroying <%= Google::StringUtils.underscore(obj.name) -%>' | ||
| VCR.use_cassette('destroy_<%= Google::StringUtils.underscore(obj.name) -%>') do | ||
| run_example('delete_<%= Google::StringUtils.underscore(obj.name) -%>') | ||
| end | ||
| 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ooh, interesting. Several parts of this I have a response to, the rest I'm still chewing on.
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.
This isn't great! We have some 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
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.
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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
But I see some issues with your methodology as well:
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 Downside: that sounds hard to do.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally understand the issues. I don't entirely follow your solution |
||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| <% if false # the license inside this if block assertains to this file -%> | ||
| # Copyright 2017 Google Inc. | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| <% end -%> | ||
| <%= compile 'templates/license.erb' -%> | ||
|
|
||
| <%= lines(autogen_notice :ruby) -%> | ||
|
|
||
| function gcompute_health_check_ref(String $name, String $project_name) >> String { | ||
| "https://www.googleapis.com/compute/v1/projects/$project_name/global/healthChecks/$name" | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.