Add a describe_on_supported_os DSL

[ekohl]

It is quite tedious to write the on_supported_os loop in every file. This defines a short hand in the RSpec DSL. To keep access to the OS facts, it is added to the metadata.

This is a bit hacky, but I'm submitting it here for consideration. The naming may also be confusing and perhaps there's a better name.

There are also limitations. It's no longer possible to pass a select number of operating systems.

The goal is to replace

describe 'myclass' do
  on_supported_os.each do |os, os_facts|
    context "on #{os}" do
      let(:facts) { os_facts}

      ...
    end
  end
end

With

describe_on_supported_os 'myclass' do
  ...
end

[ekohl]

I added the example here just so you could easily compare it to the block above it.

[codecov]

Codecov Report

Merging #132 (8341777) into master (563ce3d) will decrease coverage by 2.78%.
The diff coverage is 44.44%.

❗ Current head 8341777 differs from pull request most recent head d586583. Consider uploading reports for the commit d586583 to get more accurate results

@@            Coverage Diff             @@
##           master     #132      +/-   ##
==========================================
- Coverage   96.20%   93.41%   -2.79%     
==========================================
  Files           2        2              
  Lines         158      167       +9     
==========================================
+ Hits          152      156       +4     
- Misses          6       11       +5     

Impacted Files	Coverage Δ
lib/rspec-puppet-facts.rb	96.29% <44.44%> (-3.06%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 563ce3d...d586583. Read the comment docs.

[jcbollinger]

I do not care for the proposed addition, but I don't mind it being added as long as I can opt out of using it. That includes it not being incorporated into the standard PDK test templates, though I suppose PDK is out of scope here.

I find the proposed new approach less readable overall than the current one, the (minor, as far as I am concerned) advantage of reducing nesting depth notwithstanding. Additionally, I see some functionality issues, including:

• I routinely write multiple test cases (i.e. context blocks) for each entity under test. The proposed approach seems to achieve its second level of nesting reduction by doing away with context blocks, so I guess I would need to implement multiple test cases by using multiple describe_on_supported_os loops. But then I don't have distinct labels for the different cases, and I have nowhere to hang code that applies to more than one of them (preconditions or shared examples, for instance).

• I sometimes write tests involving augmented or variant fact patterns. It's well known how to do that with the current approach (via let(:facts)), but I don't see how to do it with the proposed new one.

• It's not clear from the description whether the proposed new approach still supports let(:params) or an equivalent. Not doing so would be an absolute deal-breaker.

• It's not clear from the description whether the proposed approach applies to all the kinds of Puppet entities that rspec_puppet supports -- not just classes, but also defined types, hosts, applications, etc. Some of those (functions, types) don't have much to gain from rspec-puppet-facts to begin with, so support for them is moot. But the issue that this PR claims to address is not adequately resolved if the rest are not covered.

[genebean]

Also, would something like this still be possible?

let(:facts) do
  facts.merge(classification: {
                'stage' => nil,
              })
end

[genebean]

The current setup only symbolizes part of the data used in blocks like this. How would one expanded to use non-legacy facts look? It looks like this today, for context:

case os_facts[:os]['family']
when 'RedHat'
  ...
when 'Debian'
  ...
else
  ...
end

[ekohl]

metadata[:os_facts] is os_facts so metadata[:os_facts][:os]['family'].

Again, really unrelated but yes, it's really confusing that it's not fully using symbols. The root cause is here. Ideally speaking it would use something like HashWithIndifferentAccess but pulling in ActiveSupport just for that feels very heavy. However, facts from FacterDB should really be read-only besides overrides. We can probably easily subclass Hash and override [](key) to call .to_s(key). While we're at it, implement override in it as well.

[ekohl]

Regarding overriding facts, this hasn't changed. I'd recommend https://github.com/voxpupuli/voxpupuli-test#fact-handling where I've documented what I think are best practices.

In short: you need to use super(). So:

context 'with SELinux on' do
  let(:facts) { super().merge(selinux: true) }
end

Unrelated, but with modern facts this isn't really going to work since you need to deep merge. For this we have override_facts in voxpupuli-test. #112 is open for this and override_facts() should live in this repository.