[RFC] New syntax

[rodjek]

I'm planning on introducing a new syntax in rspec-puppet 3.0.0 that encourages the use of standard rspec matchers where possible. If you're using serverspec for your acceptance tests, this will look pretty familiar.

I'm just in the planning and experimenting stage at the moment, and I'd love to hear any suggestions or feedback from the users rspec-puppet. The main change so far is making the resources in the catalogue the subjects of the test, rather than the catalogue itself.

Where currently, you might have a test like this:

describe 'mysql::service' do
  it { is_expected.to contain_service('mysql').with_ensure('running').with_enabled(true) }
end

It could instead be expressed using standard rspec matchers as:

describe 'mysql::service' do
  describe service('mysql') do
    its(:parameters) { is_expected.to include(:ensure => 'running', :enable => true) }
  end
end

By creating an expectation on an aspect of the resource, you're implicitly expecting that the resource is present in the catalogue, but you can explicitly specify this expectation by using the standard exist matcher.

describe service('mysql') do
  it { is_expected.to exist }
end

Or in the negative:

describe service('mysql') do
  it { is_expected.not_to exist }
end

The relationships between resources is still implemented with custom matchers, but they are much more lightweight than they are currently.

describe service('mysql') do
  it { is_expected.to require file('/etc/mysql/mysql.conf') }
end

By making these distinct expectations, you can now easily test negatives (for example, that a resource does not subscribe to another resource) and compound expectations:

describe service('mysql') do
  it { is_expected.to require(file('/etc/mysql/mysql.conf')).or require(file('/etc/mysql.conf')) }
end

As I said at the start, nothing here is set in stone and all suggestions are welcome and will be considered. Whatever the new syntax ends up being, it will work along side the existing syntax, so none of your existing tests will suddenly break. I do plan on deprecating the current syntax in a future major release and eventually removing it in an even later major release, but it won't be any time soon.

[coveralls]

Coverage increased (+0.2%) to 91.612% when pulling 3b0c8c2 on new_syntax into 11f93e7 on master.

[cyberious]

While I understand what you are trying to do, I believe the verbosity you are adding will actually lead to more code and confusion. For instance the its(:parameters) syntax could just as easily be the similar older pattern without the verbosity of its(:parameters) block. Perhaps a similar pattern to what has been adopted by serverspec in that we have it { is_expected.to have_ensure('running') } or multiple values with something like it { is_expected.to have( {:ensure => 'running' , :enabled => true }) }

[logicminds]

How would this new format work with method chaining?

it do
  is_expected.to contain_file('/tmp/test').with({ ensure: present}).that_comes_before("File['/tmp']")
end

[rodjek]

@cyberious 👍 I like it

@logicminds something like this

describe file('/tmp/test') do
  its(:parameters) { is_expected.to include(ensure: 'present') }
  it { is_expected.to come_before(file('/tmp')) }
end

One of the main ideas behind the new syntax is to discourage the use of long chains of methods in the expectations and instead make it easy to do small, discrete expectations on a resource. This leads to much easier to read tests and output

my::class
  File[/tmp/test]
    should be in the catalogue
    should come before File[/tmp]
    parameters
      should include {:ensure => 'present'}

[DavidS]

@rodjek adjacent to this, a question on Slack was raised on how to test for global properties of the catalog, like, "no directory with a source should have recurse set to true". Do you see a possibility to integrate such functionality here?

[rodjek]

@DavidS Yep, that's similar to another issue #229 where the user wants to be able to check that all cron resources have certain IO redirects in their command string. What I'm thinking is a way to enumerate resources of a particular type, so maybe something like this?

resources(:file).select { |f| f.ensure == 'directory' && f.source }.each do |file_resource|
  describe file_resource do
    it { is_expected.to have_attributes(:recurse => false) }
  end
end

[DavidS]

It reminds me of (global) property-based testing:

describe resource(:ifile).select { ... } do
  all { are_expected.to ... }
end

[rodjek]

Yeah, it could be written as:

describe resources(:file).select { |f| f.ensure == 'directory' && f.source } do
  it { is_expected.to all(have_attributes(:recurse => false)) }
end

That would give slightly less easy to understand output in documentation format, but a more consise test.

[ekohl]

How would this deal with resources within a module? given the current double underscore I'd expect describe concat__fragment('x') do ... end

Another thing I'd like to see is easy verification of the full content. Currently we have helper functions:

def verify_concat_fragment_contents(subject, title, expected_lines)
  is_expected.to contain_concat__fragment(title)
  content = subject.resource('concat::fragment', title).send(:parameters)[:content]
  expect(content.split("\n") & expected_lines).to match_array(expected_lines)
end

def verify_concat_fragment_exact_contents(subject, title, expected_lines)
  is_expected.to contain_concat__fragment(title)
  content = subject.resource('concat::fragment', title).send(:parameters)[:content]
  expect(content.split(/\n/).reject { |line| line =~ /(^#|^$|^\s+#)/ }).to match_array(expected_lines)
end