(PDK-511) Add canonicalization checking if puppet strict is on. #30
Conversation
lib/puppet/resource_api.rb
Outdated
| when :warning | ||
| Puppet.warning(message) | ||
| when :error | ||
| # TODO: get more context for the abort error message |
There was a problem hiding this comment.
What's the current output of, say, puppet resource when this is hit, and what should it be?
spec/puppet/resource_api_spec.rb
Outdated
| @@ -408,105 +408,308 @@ def set(_context, changes) | |||
| before(:each) do | |||
| stub_const('Puppet::Provider::Canonicalizer', Module.new) | |||
| stub_const('Puppet::Provider::Canonicalizer::Canonicalizer', provider_class) | |||
| Puppet.settings[:strict] = :off | |||
There was a problem hiding this comment.
Set a global default using https://relishapp.com/rspec/rspec-core/docs/hooks/before-and-after-hooks#define-%60before%60-and-%60after%60-blocks-in-configuration . That way we don't have to worry about puppet changing under our feet across versions, and it becomes clearer if and when individual tests have to override.
And the default should be :error, because we need to be able to work under the strictest setting without crashing and burning.
spec/puppet/resource_api_spec.rb
Outdated
| end | ||
|
|
||
| it { expect { described_class.register_type(definition) }.not_to raise_error } | ||
| context 'with get values that will not be canonicalized' do |
There was a problem hiding this comment.
I'm having a hard time parsing this. Maybe "when get returns canonical values" ?
spec/puppet/resource_api_spec.rb
Outdated
| it('returns an array of TypeShims') { expect(type.instances[0]).to be_a Puppet::ResourceApi::TypeShim } | ||
| it('its name is set correctly') { expect(type.instances[0].name).to eq 'somename' } | ||
| end | ||
| context 'with get values that will be canonicalized' do |
There was a problem hiding this comment.
As above, maybe "when canonicalize changes get values"?
spec/puppet/resource_api_spec.rb
Outdated
| end | ||
|
|
||
| it { expect { described_class.register_type(definition) }.not_to raise_error } | ||
| context 'with get values that will not be canonicalized' do | ||
| it { expect { described_class.register_type(definition) }.not_to raise_error } |
There was a problem hiding this comment.
This test does not exercise the get/canonicalize interaction, so you can leave it in the common section.
spec/puppet/resource_api_spec.rb
Outdated
| allow(type.my_provider).to receive(:get) | ||
| .with(kind_of(Puppet::ResourceApi::BaseContext)) | ||
| .and_return([{ name: 'somename', test_string: 'canonfoo' }, | ||
| { name: 'other', test_string: 'canonbar' }]) |
There was a problem hiding this comment.
This is duplicating a big chunk of setup code with the new with get values that will be canonicalized section, without extracting the common parts. It would be easier to read if you do something like this:
before(:each) do
allow(type.my_provider).to receive(:get)
.with(kind_of(Puppet::ResourceApi::BaseContext))
.and_return([{ name: 'somename', test_string: first_test_string },
{ name: 'other', test_string: second_test_string }])
end
context 'with get values that will not be canonicalized' do
let(:first_test_string) { "canonfoo" }
let(:first_test_string) { "canonbar" }
# ...
end
context 'with get values that will be canonicalized' do
let(:first_test_string) { "foo" }
let(:first_test_string) { "bar" }
# ...
end
spec/puppet/resource_api_spec.rb
Outdated
| end | ||
|
|
||
| after(:each) do | ||
| log_sink.clear |
There was a problem hiding this comment.
due to rspec's semantics, the code gets a new let(:log_sink) instance for each example (it), so no need to clean up log_sink.
What I do wonder though is whether Puppet::Util::Log.newdestination will be happy with all the LogCollectors accumulating? With https://github.com/puppetlabs/puppet-resource_api/pull/29/files#diff-93830fa29d616f7c87903d08b5b1b29aR34 we have https://github.com/puppetlabs/puppetlabs_spec_helper/blob/4271c7222e3366c57863bc8a77ccc117f5d6ec7d/lib/puppetlabs_spec_helper/puppet_spec_helper.rb#L134-L160 available, which could supersede this entire section here.
spec/puppet/resource_api_spec.rb
Outdated
| Returned values: {:name=>"somename", :test_string=>"foo"} | ||
| Canonicalized values: {:name=>"somename", :test_string=>"canonfoo"} | ||
| MESSAGE | ||
| end |
There was a problem hiding this comment.
I'm not understanding what the different values here are used/useful for. Let's work on this bit after lunch?
The multiple layers of log_sinks were stepping on each other
No description provided.