Performance improvements

[ekohl]

This includes some performance improvements that reduce the loading of tests by about 4 seconds (from ~11 to ~7 seconds) so about a third.

There are some gotchas. Each commit captures what it does. Copying for readability.

f39009f Move facterversion_obj declaration out of the loop

There's a very good chance we'll need this object and this saves parsing it over and over.

This is a safe patch.

5452fe9 Do not query for the exact facter version

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.

Actual results will vary on the FacterDB. Note that a new Facter version will trigger the bad code unless all operating systems are updated so this is a very likely code path.

It also makes the easier to read.

This appears to be backwards incompatible. I didn't expect it, but it looks like you can query for 3.1.2 and get back 3.1.6 in the current releases while I assumed that was impossible.

4d80729 Collect facts iteratively

Rather than getting the facts for every filter spec and then discarding that, this stores the found facts in an array and uses that.

This eliminates a call to FacterDB::get_facts with a very complex filter.

A quick test in puppet-nginx reduces loading of tests by about 2 seconds (from ~9 to ~7).

This should be fairly safe. The order of the results is now likely to be different and perhaps that's causing test failures.

Fixes #123

[ekohl]

The implied question was: is this a good path forward? If so, I can take a look at finishing them up.

[ghoneycutt]

Great work @ekohl !!

Move facterversion_obj declaration out of the loop

I read this as a safe way to improve existing code. +1

Do not query for the exact facter version

I'm understanding this as we can currently test for a specific version of Facter and after this patch, we cannot. Very likely I'm totally misunderstanding this, though if that is the case, I would not want to do that or would want an option where you can bypass it and specifically test a given version of Facter.

Collect facts iteratively

I read this as another optimization so +1. I'm not sure what the ordering that potentially causing test failures means and what the implications are for that.

[ghoneycutt]

Good idea using the nginx module for testing. Would suggest before merging to pick a handful of popular VP modules that are in a good state already and see if this has any unintended consequences.

[ekohl]

I'm understanding this as we can currently test for a specific version of Facter and after this patch, we cannot. Very likely I'm totally misunderstanding this, though if that is the case, I would not want to do that or would want an option where you can bypass it and specifically test a given version of Facter.

It should still work like that (since Gem::Version.new(facts[:facterversion]) <= Gem::Version.new(facterversion) should also yield an exact match). However, when I ran the tests locally they failed. For example:

https://github.com/mcanevet/rspec-puppet-facts/blob/c2987a1b6cd7a9ab104aed8aa7ff61e28ce29cce/spec/rspec_puppet_facts_spec.rb#L738-L753

The queried facterversion is 3.1.2 but it expected 3.1.6 to be returned. After the second patch it returns a set of facts with version 3.0.2 since 3.1.6 > 3.1.2. I wonder if this is silently converted to a float in the filterspec by JGrep.

Given there was an explicit test for it, that makes me think it's a regression.

[coveralls]

Coverage decreased (-0.009%) to 99.338% when pulling 4d80729 on ekohl:performance-improvements into c2987a1 on mcanevet:master.

[ghoneycutt]

If you specify 3.1.2, I would expect it to respond with the same 3.1.2 or an error.

[ekohl]

You get the error if you specify strict versions, but that's not the default behavior.

[sanfrancrisko]

Just so I'm following correctly @ekohl , do you think the test expectations (that you referred to here need updated given the change in behaviour change from this PR?

[ekohl]

@sanfrancrisko at this point I'm uncertain what is expected. I'd like some guidance from the maintainers what the behavior should be. That means either changing the tests (as I suggested) or changing the code to pass the tests. Until I get that feedback I'm blocked.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.07 ⚠️

Comparison is base (0921dff) 94.93% compared to head (da296b9) 94.87%.

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #124      +/-   ##
==========================================
- Coverage   94.93%   94.87%   -0.07%     
==========================================
  Files           2        2              
  Lines         158      156       -2     
==========================================
- Hits          150      148       -2     
  Misses          8        8              

Impacted Files	Coverage Δ
lib/rspec-puppet-facts.rb	98.01% <100.00%> (-0.03%)	⬇️

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

[ekohl]

I've decided to split up the patches for a better changelog (and easier discussion): #151 & #152

[ekohl]

All work is merged, except #152.