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

cmp not treating 0 as integer only as string #991

Merged
merged 3 commits into from
Aug 30, 2016

Conversation

jeremymv2
Copy link
Contributor

@jeremymv2 jeremymv2 commented Aug 29, 2016

This change allows both tests to pass, which I believe is the intent of the cmp matcher.

describe 0 do
  it { should cmp 0 }
  it { should cmp '0' }
end

Without the change from this PR, it fails one of the tests:

~/Devel/ChefProject/inspec (cmp_treat_0_as_integer $%=)$ inspec exec ../tmp/cmp_test.rb --format=documentation

0
  should cmp 0
  should cmp "0" (FAILED - 1)

Failures:

  1) 0 should cmp "0"
     Failure/Error: DEFAULT_FAILURE_NOTIFIER = lambda { |failure, _opts| raise failure }

       expected: "0"
            got: 0

       (compared using `cmp` matcher)
     # ../tmp/sshd_test.rb:7:in `block (2 levels) in load'

Finished in 0.01047 seconds (files took 0.79418 seconds to load)
2 examples, 1 failure

Failed examples:

rspec  # 0 should cmp "0"

~/Devel/ChefProject/inspec (cmp_treat_0_as_integer $%=)$

@@ -232,7 +232,7 @@
RSpec::Matchers.define :cmp do |first_expected|

def integer?(value)
!(value =~ /\A[1-9]\d*\Z/).nil?
!(value =~ /\A[0-9]\d*\Z/).nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

Great catch and really good test-case. We left 0 out, to cover the detection for octal numbers. Therefore 0 is only valid if the length of the string is 1

@jeremymv2
Copy link
Contributor Author

@chris-rock assuming that last commit passes tests, could you lend 👀 on this please to ensure it's sane for what how you expect 'cmp' to work?

@chris-rock
Copy link
Contributor

This is a great fix. Thanks for finding this edge case @jeremymv2

@chris-rock chris-rock merged commit 4673dfd into inspec:master Aug 30, 2016
@chris-rock chris-rock modified the milestone: 0.33.0 Sep 2, 2016
@chris-rock chris-rock added the Type: Bug Feature not working as expected label Sep 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Feature not working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants