Restructure tests

[chrekh]

Replace multiple is_expected.to contain_file() for the same file with a single
is_expected.to contain_file() with instead multiple .with_content() or .without_content()
for improved readability and preformance.

[igalic]

this looks much clearer, indeed
looking forward to seeing the time difference on Travis

[ekohl]

Looks like it's not that big a difference because tests were already quick. Still a win since it's easier to read.

[alexjfisher]

Looks like it's not that big a difference because tests were already quick. Still a win since it's easier to read.

I'm still fairly convinced that rspec-puppet caches catalogs (where params and facts haven't changed) and wasn't recompiling for every example.

[ekohl]

Something I always wonder about is whether you want to copy these to be explicit or have some variable in rspec with this location and reuse the checks for all operatingsystems. Then if you need OS specific checks, only list those. So roughly:

let(:keys_file) do
  case facts[:osfamily]
  when 'ArchLinux'
    '/etc/chrony.keys'
  # ...
  end
end

Then later on it { is_expected.to contain_file(keys_file).with_... }

That's the approach I take most often because it's less work code but is it a good practice?

[chrekh]

I'm not qualified to answer that. But I like it.
I also believe that the separate blocks for context 'with some params passed in' do now could be replaced with a single, since we now don't have separate template files.

[ekohl]

Given these are now the same, you can pull it outside of the case facts[:osfamily] statement to reduce the duplication further. I'd suggest to also move context 'using defaults' outside of the case statement. Then the case statement is just for the config_file.

[chrekh]

If you mean replace the blocks

          it do
            is_expected.to contain_file(keys_file)
              .with_mode('0644')
              .with_owner('0')
              .with_group('0')
              .with_replace(true)
              .with_content("0 xyzzy\n")
          end

with a single block, that's not possible since the mode differs between osfamilies.

[ekohl]

So this is sort of where it may break down. If you copy the block, a variable for keys_file isn't that useful. You can either add a keys_file_mode variable. Another is to question whether they really need to differ. For example, I wonder if a keys file should be world readable.

[chrekh]

Yes the mode should probably default to 0640 for all osfamilies. But I think it kind of still make sense to have a case statement for both the keyfile and configfile for the using defaults test, since the defaults is in fact set by param.pp different on each osfamily, and the might change some time. The with some params passed in on the other hand don't need to.

[ekohl]

One possible solution:

context 'using defaults' do
  it do
    is_expected.to contain_file(keys_file)
              .with_owner('0')
              .with_group('0')
              .with_replace(true)
              .with_content("0 xyzzy\n")
  end

  case facts[:os_family]
  when 'ArchLinux'
    # config_file here
    it { is_expected.to contain_file(keys_file).with_mode('0644') }
  # other distros
  end
end

That way you can clearly see the common parts and those that differ. Now this may be slower if it needs to be compiled multiple times (@alexjfisher suggested it doesn't, haven't benchmarked myself, only feels like it).

Of course unifying them all as much as possible is the easiest for maintenance.

[chrekh]

Do you mean like ad65d22? No that didn't work either, also group differs between osfamilies.

[chrekh]

This is no longer current or relevant.