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

Fix Inspec::Attribute.to_ruby and add unit test #3773

Merged
merged 3 commits into from Jan 31, 2019

Conversation

Projects
None yet
4 participants
@james-stocks
Copy link
Contributor

james-stocks commented Jan 30, 2019

#3756 introduced a regression, in which calling to_ruby on an Attribute object would throw an exception because it calls a method, default, which was renamed.

This PR fixes that.

Signed-off-by: James Stocks jstocks@chef.io

Fix Inspec::Attribute.to_ruby and add unit test
Signed-off-by: James Stocks <jstocks@chef.io>

@james-stocks james-stocks force-pushed the james-stocks/attribute_to_ruby branch from 716cd46 to 5868eb5 Jan 30, 2019

@james-stocks james-stocks changed the title Fix Insec::Attribute.to_ruby and add unit test Fix Inspec::Attribute.to_ruby and add unit test Jan 30, 2019

@james-stocks james-stocks requested a review from clintoncwolfe Jan 30, 2019

@clintoncwolfe
Copy link
Contributor

clintoncwolfe left a comment

Great work! I think 2 things are needed:

  1. We need to add default: to the gerneated code, along with the value:
  2. I think we should probably attempt to eval (or just compile) the code in the unit test.

Thanks!

@@ -93,7 +93,7 @@ def to_hash
def to_ruby
res = ["#{ruby_var_identifier} = attribute('#{@name}',{"]
res.push " title: '#{title}'," unless title.to_s.empty?
res.push " default: #{default.inspect}," unless default.to_s.empty?
res.push " value: #{value.inspect}," unless value.to_s.empty?

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Jan 30, 2019

Contributor

Do you expect profiles generated by this to work on older InSpecs? If so, you should add an additional line:

res.push "  default: #{value.inspect}," unless value.to_s.empty?

This way any generated code will contain both a value: and a default: option; newer InSpecs will use value:, and older inspecs will use default:

This comment has been minimized.

@james-stocks

james-stocks Jan 30, 2019

Author Contributor

Yes it will have to work on older InSpec versions to not be a breaking change. Both older and current InSpec should be good with the extra key being passed, right?

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Jan 30, 2019

Contributor

Yes.

describe 'to_ruby method' do
it 'generates the code for the attribute' do
attribute = Inspec::Attribute.new('application_port', description: 'The port my application uses', value: 80)
attribute.to_ruby.must_equal <<-RUBY.chomp

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Jan 30, 2019

Contributor

This is just a preference, but I find it is easier to maintain (as well as handle failures) when the individual things you are looking for are split out. It also makes it less susceptible to whitespace changes. Like this:

ruby_code = attribute.to_ruby
ruby_code.must_include "attribute('application_port'" # Should have the declaration
ruby_code.must_include 'value: 80' # We care that we use the 'value' attribute
ruby_code.must_include 'default: 80' # We care that we use the 'default' attribute

This comment has been minimized.

@james-stocks

james-stocks Jan 30, 2019

Author Contributor

I think it's generally better to not get tripped up by whitespace changes - but in this case isn't correct whitespacing part of the responsibility of the method?

description: 'The port my application uses',
})
RUBY
end

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Jan 30, 2019

Contributor

I think we might also want to try to eval the code. We care that we not only generate lines with certain things in it, but that code be valid code.

This comment has been minimized.

@james-stocks

james-stocks Jan 30, 2019

Author Contributor

That sounds good. It looks like eval always returns nil and all I can test is that nothing raises.
It also looks like MiniTest has no assertion for "must_not_raise" and all I can do is call eval() bare and let its unhandled exception fail the test?

Let me know if you had a better approach in mind.

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Jan 30, 2019

Contributor

Yes, that's how it works - nothing is raised; and then I usually put a test afterwards, to ensure it worked.

This comment has been minimized.

@james-stocks

james-stocks Jan 30, 2019

Author Contributor

Awesome! Thank you!

clintoncwolfe added some commits Jan 30, 2019

Also issue default: option for backwards compatibility
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Add eval to unit tests; break up codegen test into individual elements
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>

@clintoncwolfe clintoncwolfe requested review from miah and jerryaldrichiii Jan 30, 2019

I am now a contributor

@jerryaldrichiii
Copy link
Contributor

jerryaldrichiii left a comment

Looks good to me! Thanks for your feedback @clintoncwolfe.

@james-stocks
Copy link
Contributor Author

james-stocks left a comment

+1 to Clinton's improvements on top of my initial attempt

@miah

miah approved these changes Jan 30, 2019

@clintoncwolfe clintoncwolfe merged commit ee2249c into master Jan 31, 2019

4 checks passed

DCO This commit has a DCO Signed-off-by
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details

@james-stocks james-stocks deleted the james-stocks/attribute_to_ruby branch Jan 31, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment