Skip to content
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

New 'be_in' matcher for matching against values in a list #2022

Merged
merged 6 commits into from
Aug 7, 2017

Conversation

rx294
Copy link
Contributor

@rx294 rx294 commented Jul 18, 2017

Fixes #2018

Signed-off-by: Rony Xavier rx294@nyu.edu

rx294 and others added 2 commits July 18, 2017 12:08
Fixes inspec#2018

Signed-off-by: Rony Xavier <rx294@nyu.edu>
Signed-off-by: Aaron Lippold <lippold@gmail.com>
describe nginx do
   its(module_list) { should be_in AUTHORIZED_MODULE_LIST }
end
Fixes inspec#2018

Signed-off-by: Rony Xavier <rx294@nyu.edu>
Fixes inspec#2018

Signed-off-by: Rony Xavier <rx294@nyu.edu>
Copy link
Contributor

@chris-rock chris-rock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great addition @rx294 Lets just add integration tests!

@@ -78,6 +78,40 @@
end

it 'constructs a simple resource+argument with method calls' do
obj.qualifier = [['resource'], ['hello', 'world']]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those tests are great, but they are not testing the be_in matcher. Instead they only verify that the inspec obects code generates the right InSpec ruby code. Instead I propose to create a new test/integration/matcher_test.rb. This will run RSpec and test the matcher. See https://github.com/chef/inspec/blob/master/test/integration/default/cmp_matcher_spec.rb

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @chris-rock, I misunderstood. I will add the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chris-rock I have pushed up test/integration/matcher_test.rb for the 'be_in' matcher with tests that mirror the cmp matcher tests.
Please take a look.
Thank you

Signed-off-by: Rony Xavier <rx294@nyu.edu>
Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rx294 this is a great addition. The travis failure is unrelated.

I just have some Ruby style things I'd like to see fixed before I approve this. Thank you for your contribution!

RSpec::Matchers.define :be_in do |list|
match do |item|
# Handle both single item and array
item.is_a?(Array) ? (item-list).empty? : list.include?(item)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normal Ruby styling includes a space around operators. Can you please make this (item - list) so it's clearer?


match_when_negated do |item|
# Handle both single item and array
item.is_a?(Array) ? (item&list).empty? : !list.include?(item)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normal Ruby styling includes a space around operators. Can you please make this (item & list) so it's clearer?


failure_message_when_negated do |item|
if item.is_a?(Array)
"expected `#{item}` not to be in the list: `#{list}` \nComm:\n #{(item&list)}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normal Ruby styling includes a space around operators. Can you please make this (item & list) so it's clearer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @adamleff I have made the corrects.

Signed-off-by: Rony Xavier <rx294@nyu.edu>
@adamleff adamleff changed the title New matcher 'be_in' New 'be_in' matcher for matching against values in a list Aug 3, 2017
@adamleff adamleff added the Type: Enhancement Improves an existing feature label Aug 3, 2017
Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @rx294 - thanks!

@adamleff adamleff requested a review from a team August 3, 2017 03:02
Copy link
Contributor

@chris-rock chris-rock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @rx294 for this great improvement


INSTALLED_MODULES = ['module1', 'module2', 'module3', 'module4', 'module5']

describe 'module1' do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is awesome! Thank you @rx294

@chris-rock chris-rock merged commit 041f64a into inspec:master Aug 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Improves an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants