Closed ekohl closed 2 years ago
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 versiondescribe
without receiver is a (default) option, but it will be removed in favour of RSpec.describe
:if
/:unless
will be removedA more canonical RSpec way to write such a spec would be:
specify { expect(package('httpd')).to be_installed } if os[:family] == 'redhat'
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.
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.
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 https://github.com/voxpupuli/puppet-trusted_ca/pull/44 to try out this new syntax in a rather trivial module.
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
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?
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
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!
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
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
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.
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.
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.
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.
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!
You're very welcome.
Closing the loop: https://github.com/voxpupuli/puppet-redis/pull/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.
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?
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.
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:
This is using serverspec's
command
and verifies the exit code is 0. Somehow this triggersRSpec/RepeatedExampleGroupBody
but they're clearly different. Is this a bug?