RSpec/RepeatedExampleGroupBody false positive

[ekohl]

We have the following code: https://github.com/voxpupuli/puppet-trusted_ca/blob/428fa7425a69aa61bd7a18201a4b0bc876eddc50/spec/acceptance/certs_spec.rb#L47-L53

Copied here for an easier overview:

describe 'trusted_ca' do
  # ... other code ...

  describe command("/usr/bin/curl https://#{fact('hostname')}.example.com:443") do
    its(:exit_status) { is_expected.to eq 0 }
  end

  describe command("cd /root && /usr/bin/java SSLPoke #{fact('hostname')}.example.com 443") do
    its(:exit_status) { is_expected.to eq 0 }
  end
end

This is using serverspec's command and verifies the exit code is 0. Somehow this triggers RSpec/RepeatedExampleGroupBody but they're clearly different. Is this a bug?

[pirj]

Thanks for reporting. Quite an interesting case.

The body, which in RuboCop's terms is the block passed to describe, of those two example groups is identical.

I see that using this DSL is common for beaker-rspec:

describe file('/voxpupuli-acceptance-test') do
    it { is_expected.to be_file }

RSpec's example group methods arguments doc suggests it should be a string:

      #   @overload $1(doc_string, *metadata, &example_implementation)
      #     @param doc_string [String] The group's doc string.

However, it doesn't have to.

beaker-rspec's DSL relies on the fact that the implicit subject is that docstring unless docstring is a class.

This DSL, even if quite widespread in Puppet ecosystem, is not something RuboCop RSpec can flawlessly work with.

Random rant:

require 'spec_helper'

describe package('httpd'), :if => os[:family] == 'redhat' do
  it { should be_installed }
end

This code (https://serverspec.org/), even though it looks readable, is slightly outdated in terms of RSpec:

• should is deprecated and will be removed in a major RSpec version
• describe without receiver is a (default) option, but it will be removed in favour of RSpec.describe
• :if/:unless will be removed
• doc_string abuse

A more canonical RSpec way to write such a spec would be:

specify { expect(package('httpd')).to be_installed } if os[:family] == 'redhat'

[ekohl]

A more canonical RSpec way to write such a spec would be:

That's interesting. I think that does look more readable since it's closer to regular Ruby, rather than really a DSL.

That said, I do wonder about this case where you want to test multiple properties. For example:

describe service('httpd') do
  it { is_expected.to be_running }
  it { is_expected.to be_enabled }
end

You would end up with a bunch of duplication. It is quite verbose.

[pirj]

How would you like:

specify do
  expect(service('httpd'))
    .to be_running
    .and be_enabled
end

Additionally, it's just one example instead of two. And if there's something heavy setup going on, it will only perform it once, and will make two checks against it.

edit: corresponding documentation

[ekohl]

This issue has quickly become way more enlightening than I expected. I greatly appreciate it.

How would you like:

That looks very nice. I was not aware that this was possible.

So back to the original problem: how would I rewrite the commands listed in my initial message. They're using rspec-its but looking at the matcher documentation I suppose have_attributes would be the correct matcher.

I've opened voxpupuli/puppet-trusted_ca#44 to try out this new syntax in a rather trivial module.

[pirj]

With have_attributes, you'll still have the same body in those examples:

  describe command("/usr/bin/curl https://#{fact('hostname')}.example.com:443") do
    it { is_expected.to have_attrubutes(exit_status: 0) }
  end

  describe command("cd /root && /usr/bin/java SSLPoke #{fact('hostname')}.example.com 443") do
    it { is_expected.to have_attrubutes(exit_status: 0) }
  end

The problem is in the first argument to describe that sets the subject that is later used by is_expected (which is a shorthand for expect(subject)).

I'd suggest:

specify do
  expect(command("/usr/bin/curl https://#{fact('hostname')}.example.com:443"))
    .to have_attrubutes(exit_status: 0)
end

or:

def execute_without_errors
  have_attrubutes(exit_status: 0)
end

specify do
  expect(command("/usr/bin/curl https://#{fact('hostname')}.example.com:443"))
    .to execute_without_errors
end

[ekohl]

I'd suggest:

This is indeed what I thought. I already took the specify { ... } change in mind.

The only thing I now run into is that it runs the specify blocks before applying the Puppet manifest. I've often struggled to understand rspec ordering. Would using order: :defined as suggested on https://relishapp.com/rspec/rspec-core/docs/configuration/overriding-global-ordering help here?

[pirj]

Where is the manifest applied exactly?

Would using order: :defined help

It might, actually. It changes the order of executed examples.
But there should be a better approach, like applying the manifest in a before hook.

  describe 'success after cert' do
    let(:manifest) { <<-EOS }
      class { 'trusted_ca': }
      trusted_ca::ca { 'test': source => '/etc/ssl-secure/test.crt' }
    EOS

    before { apply_manifest(manifest, catch_failures: true) }

    it 'works idempotently with no errors' do
      # Run it for the second time and test for idempotency
      apply_manifest(manifest, catch_changes: true)
    end

    specify do
      expect(package('ca-certificates')).to be_installed
    end

    def execute_with_no_errors
      have_attributes(exit_status: 0)
    end

    specify do
      expect(command("/usr/bin/curl https://#{fact('hostname')}.example.com:443"))
        .to execute_with_no_errors
    end

    specify do
      expect(command("cd /root && /usr/bin/java SSLPoke #{fact('hostname')}.example.com 443"))
        .to execute_with_no_errors
    end
  end

[ekohl]

Where is the manifest applied exactly?

I wrote a shared example in a gem so that we don't need to maintain it in our 100+ modules:
https://github.com/voxpupuli/voxpupuli-acceptance/blob/2107b0f62067a73b6b3f409f073f5c025cc59c9d/lib/voxpupuli/acceptance/examples.rb#L1-L11

    let(:manifest) { <<-EOS }
      class { 'trusted_ca': }
      trusted_ca::ca { 'test': source => '/etc/ssl-secure/test.crt' }
    EOS

Does this syntax work? That would be new to me, but it looks cleaner than what's currently used.

    before { apply_manifest(manifest, catch_failures: true) }

What if this fails? Even applying it the first time can be considered a test. On the other hand, you are right in that it really is also a precondition to all other tests.

I really appreciate the in depth responses. If we ever do meet, I owe you a drink or 2!

[pirj]

Does this syntax work?

Yes. A less mind-blowing option is:

let(:manifest) do
  <<-EOS
    class { 'trusted_ca': }
    trusted_ca::ca { 'test': source => '/etc/ssl-secure/test.crt' }
  EOS
end

[pirj]

I wrote a shared example

  it 'applies with no errors' do
    apply_manifest_on(host, manifest, catch_failures: true)
  end

  it 'applies a second time without changes' do
    apply_manifest_on(host, manifest, catch_changes: true)
  end

Does the order matter here? I see that options are different.

I'm wondering why, and how those examples have shared state.
Typically, all examples should run from the clean slate. Shared state is not recommended unless strictly necessary.
https://rspec.rubystyle.guide/#avoid-hooks-with-context-scope

https://relishapp.com/rspec/rspec-rails/v/5-0/docs/transactions

[pirj]

        before { apply_manifest(manifest, catch_failures: true) }

What if this fails? Even applying it the first time can be considered a test.

Hooks are considered to be a part of the example. If a hook fails - the example fails, too.

I really appreciate the in depth responses. If we ever do meet, I owe you a drink or 2!

You're welcome! Happy to help streamline specs for the whole Vox Populi.

[ekohl]

Yes. A less mind-blowing option is:

That's the one I usually use.

Does the order matter here? I see that options are different.

Yes. The first application is allowed to have changes, but no failures. On the second time it may not have changes (otherwise it would not be idempotent).

Very often "apply this manifest and ensure it's idempotent" is the biggest part of the test for us. The other parts (serverspec's package, file, service, command, etc resources) then add some additional validation that it did the right thing (like using curl to verify Apache works).

Typically, all examples should run from the clean slate. Shared state is not recommended unless strictly necessary.

I'm not really sure that's feasible. This is running on real systems (often VMs or containers) where we sometimes count the setup time in minutes. Also, all the serverspec tools don't affect the system (unless a command is used to modify something). In fact, some people use it to verify production systems. It's just that we added a lot on top to run manifests, which for Vox Pupuli is the most relevant part.

[pirj]

The first application is allowed to have changes, but no failures. On the second time it may not have changes (otherwise it would not be idempotent).

Those two examples should be wrapped in an example group with order: :defined. Otherwise, they may be run in a reverse order.

[pirj]

Do you think there is anything actionable for rubocop-rspec?

The discussion is more RSpec-related, I suggest us to move to RSpec's mailing list.

[ekohl]

I think for now there is nothing actionable here. It's been very enlightening and I'll have to see about changing the style we use in our tests. Thanks for the insights!

[pirj]

You're very welcome.

[ekohl]

Closing the loop: voxpupuli/puppet-redis#438 is the first place I tried out these lessons. I'm not sure I like the negating style. You have to be quite verbose to combine things. Otherwise it certainly looks better.

[pirj]

Glad I could help. Do you mind if I comment there on the PR?
I'm quite concerned regarding the expect(service(redis_name)).not_to be_enabled.and be_running syntax. I was believing RSpec raises an exception in this case. How does it work?

[ekohl]

Oh please do comment, I welcome your insight. I was also not entirely sure about that line myself. It does appear to work because when it failed (I used to instead of not_to) it did properly report.