Open ekohl opened 3 years ago
Merging #132 (8341777) into master (563ce3d) will decrease coverage by
2.78%
. The diff coverage is44.44%
.:exclamation: 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%) |
:arrow_down: |
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.
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.
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. https://github.com/voxpupuli/rspec-puppet-facts/issues/112 is open for this and override_facts()
should live in this repository.
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
With