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

Enable and fix rubocop violations #179

Open
rski opened this issue Sep 5, 2016 · 2 comments
Open

Enable and fix rubocop violations #179

rski opened this issue Sep 5, 2016 · 2 comments

Comments

@rski
Copy link
Member

rski commented Sep 5, 2016

As of ea7cac4 some checks are disabled. These have to be enabled and fixed.

@alexjfisher
Copy link
Member

have to? I'm not so sure (but would be happy to be convinced otherwise). I did consider fixing the violations, but couldn't think of a nice way that actually improved the tests.

eg in

it 'writes the values in order' do # rubocop:disable RSpec/MultipleExpectations
  expect(content.scan(%r{^\s*post-down .*$})[0]).to eq('    post-down /bin/touch /tmp/eth1-down1')
  expect(content.scan(%r{^\s*post-down .*$})[1]).to eq('    post-down /bin/touch /tmp/eth1-down2')
end

From http://betterspecs.org/#single

In isolated unit specs, you want each example to specify one (and only one) behavior. Multiple expectations in the same example are a signal that you may be specifying multiple behaviors.

I think it could be argued that we're only testing one behaviour ie 'values are in order'

From http://programmers.stackexchange.com/questions/7823/is-it-ok-to-have-multiple-asserts-in-a-single-unit-test

Tests should fail for one reason only, but that doesn't always mean that there should be only one Assert statement. IMHO it is more important to hold to the "Arrange, Act, Assert" pattern.

The key is that you have only one action, and then you inspect the results of that action using asserts. But it is "Arrange, Act, Assert, End of test". If you are tempted to continue testing by performing another action and more asserts afterwards, make that a separate test instead.

I am happy to see multiple assert statements that form parts of testing the same action...

In summary, I don't think the test would have been improved if I'd changed it to

describe 'ordering of values' do
  it 'the first value comes first' do
    expect(content.scan(%r{^\s*post-down .*$})[0]).to eq('    post-down /bin/touch /tmp/eth1-down1')
  end
  it 'the second value comes second'
    expect(content.scan(%r{^\s*post-down .*$})[1]).to eq('    post-down /bin/touch /tmp/eth1-down2')
  end
end

But maybe I just couldn't think of the right words to use. Replace 'the first value comes first' with something more meaningful and maybe I'll change my mind! :)

@rski
Copy link
Member Author

rski commented Sep 6, 2016

Ah, good point. If there is no meaningful semantics there is no point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants