voxpupuli / rspec-puppet-facts

Simplify your unit tests by looping on every supported Operating System and populating facts.
Apache License 2.0
61 stars 53 forks source link

Do not query for the exact facter version #151

Closed ekohl closed 11 months ago

ekohl commented 1 year ago

This assumes that no exact match can be found in most cases. This saves a call to FacterDB. Some testing on puppet-nginx this reduced the time for a dry run by about 1 to 2 seconds, from about ~11 to ~9 seconds.

Implementation wise this switches to using Gem::Requirement to detect matching versions. This means you can also specify ~> 3.8 as a valid facterversion, or >= 3 < 5.

This is a reworked version of a patch in https://github.com/voxpupuli/rspec-puppet-facts/pull/124 that focuses on only reworking the facterversion selection.

There are some implementation differences that can be considered backwards incompatible. Right now this is not ready to be merged, but we should talk about what is desired. Please see the test cases and their diffs for how the behavior will change.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.30 :tada:

Comparison is base (8d9370c) 94.93% compared to head (2596ab7) 95.23%.

:exclamation: Current head 2596ab7 differs from pull request most recent head d690765. Consider uploading reports for the commit d690765 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #151 +/- ## ========================================== + Coverage 94.93% 95.23% +0.30% ========================================== Files 2 2 Lines 158 168 +10 ========================================== + Hits 150 160 +10 Misses 8 8 ``` | [Impacted Files](https://app.codecov.io/gh/voxpupuli/rspec-puppet-facts/pull/151?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=voxpupuli) | Coverage Δ | | |---|---|---| | [lib/rspec-puppet-facts.rb](https://app.codecov.io/gh/voxpupuli/rspec-puppet-facts/pull/151?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=voxpupuli#diff-bGliL3JzcGVjLXB1cHBldC1mYWN0cy5yYg==) | `98.15% <100.00%> (+0.12%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

ekohl commented 1 year ago

For me it does produce more warnings. Without the patch:

No facts were found in the FacterDB for Facter v4.2.8 on {:operatingsystem=>"RedHat", :operatingsystemrelease=>"/^6/", :hardwaremodel=>"x86_64"}, using v3.14.1 instead
No facts were found in the FacterDB for Facter v4.2.8 on {:operatingsystem=>"CentOS", :operatingsystemrelease=>"/^6/", :hardwaremodel=>"x86_64"}, using v3.14.1 instead
No facts were found in the FacterDB for Facter v4.2.8 on {:operatingsystem=>"Scientific", :operatingsystemrelease=>"/^6/", :hardwaremodel=>"x86_64"}, using v3.14.1 instead

With the patch:

No facts were found in the FacterDB for Facter v4.2.11 on {:operatingsystem=>"RedHat", :operatingsystemrelease=>"/^6/", :hardwaremodel=>"x86_64"}, using v3.14.1 instead
No facts were found in the FacterDB for Facter v4.2.11 on {:operatingsystem=>"RedHat", :operatingsystemrelease=>"/^7/", :hardwaremodel=>"x86_64"}, using v4.2.13 instead
No facts were found in the FacterDB for Facter v4.2.11 on {:operatingsystem=>"RedHat", :operatingsystemrelease=>"/^8/", :hardwaremodel=>"x86_64"}, using v4.2.13 instead
No facts were found in the FacterDB for Facter v4.2.11 on {:operatingsystem=>"CentOS", :operatingsystemrelease=>"/^6/", :hardwaremodel=>"x86_64"}, using v3.14.1 instead
No facts were found in the FacterDB for Facter v4.2.11 on {:operatingsystem=>"CentOS", :operatingsystemrelease=>"/^7/", :hardwaremodel=>"x86_64"}, using v4.2.2 instead
No facts were found in the FacterDB for Facter v4.2.11 on {:operatingsystem=>"CentOS", :operatingsystemrelease=>"/^8/", :hardwaremodel=>"x86_64"}, using v4.2.2 instead
No facts were found in the FacterDB for Facter v4.2.11 on {:operatingsystem=>"Scientific", :operatingsystemrelease=>"/^6/", :hardwaremodel=>"x86_64"}, using v3.14.1 instead
No facts were found in the FacterDB for Facter v4.2.11 on {:operatingsystem=>"Scientific", :operatingsystemrelease=>"/^7/", :hardwaremodel=>"x86_64"}, using v4.2.2 instead
No facts were found in the FacterDB for Facter v4.2.11 on {:operatingsystem=>"Debian", :operatingsystemrelease=>"/^9/", :hardwaremodel=>"x86_64"}, using v4.2.2 instead
No facts were found in the FacterDB for Facter v4.2.11 on {:operatingsystem=>"Debian", :operatingsystemrelease=>"/^10/", :hardwaremodel=>"x86_64"}, using v4.2.2 instead
No facts were found in the FacterDB for Facter v4.2.11 on {:operatingsystem=>"Ubuntu", :operatingsystemrelease=>"/^16\\.04/", :hardwaremodel=>"x86_64"}, using v4.2.5 instead
No facts were found in the FacterDB for Facter v4.2.11 on {:operatingsystem=>"Ubuntu", :operatingsystemrelease=>"/^18\\.04/", :hardwaremodel=>"x86_64"}, using v4.2.5 instead
No facts were found in the FacterDB for Facter v4.2.11 on {:operatingsystem=>"FreeBSD", :operatingsystemrelease=>"/^11/", :hardwaremodel=>"amd64"}, using v4.2.1 instead
No facts were found in the FacterDB for Facter v4.2.11 on {:operatingsystem=>"FreeBSD", :operatingsystemrelease=>"/^12/", :hardwaremodel=>"amd64"}, using v4.2.4 instead
No facts were found in the FacterDB for Facter v4.2.11 on {:operatingsystem=>"Archlinux", :hardwaremodel=>"x86_64"}, using v4.2.9 instead

You could argue it's more correct that if you specify 4.2.11 that you should also get 4.2.11 facts.

Overall the (very unscientific) performance difference is:

Finished in 2.01 seconds (files took 10.32 seconds to load)

vs:

Finished in 1.55 seconds (files took 8.34 seconds to load)

So about 2 seconds or about 20% faster. https://github.com/voxpupuli/rspec-puppet-facts/pull/152 shaves off another 2 seconds and brings it down to 6.

ekohl commented 12 months ago

I'd like to see this merged. Should I merge it myself?

I wonder if this qualifies as backwards-incompatible or not. I think it isn't, though it generates more warnings. How strict should the warnings be? It looks to me that 4.2.11 != 4.2.9, but on the other hand: that's probably ok.

bastelfreak commented 12 months ago

I like the 'better safe than sorry' and suggest to merge this as backwards-incompatible. Then people won't pull it in automatically.