Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement fact memoization #122

Merged
merged 2 commits into from
Nov 9, 2020
Merged

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Sep 4, 2020

Subsequent calls to on_supported_os with the same options should result in the same facts. This can greatly speed up testing by reducing calls to FacterDB.

This currently fails, probably because it's not taking environment variables into account. I did want to submit this since it's at least a start to collaborate.

Fixes #114

@coveralls
Copy link

coveralls commented Sep 4, 2020

Coverage Status

Coverage increased (+0.03%) to 99.346% when pulling 1693e78 on ekohl:fact-memoization into 7ef35f3 on mcanevet:master.

Subsequent calls to on_supported_os with the same options should result
in the same facts. This can greatly speed up testing by reducing calls
to FacterDB.
@ekohl ekohl marked this pull request as ready for review September 7, 2020 17:39
@ekohl
Copy link
Member Author

ekohl commented Sep 7, 2020

I missed some cache keys, but I think I have them all now. Locally the tests were green so I think this is ready for review.

@ekohl
Copy link
Member Author

ekohl commented Sep 7, 2020

The build failed on out of date components. Doesn't look related to my PR, just something that changed externally and probably also shows up in master.

@ekohl
Copy link
Member Author

ekohl commented Sep 29, 2020

@mcanevet could you have a look?

@DavidS
Copy link
Contributor

DavidS commented Nov 9, 2020

added the updated agent components

@mcanevet
Copy link
Member

mcanevet commented Nov 9, 2020

@DavidS is it OK for you to merge this?

@DavidS
Copy link
Contributor

DavidS commented Nov 9, 2020

I'm running some local tests to confirm it working and being beneficial before merging.

I've also noticed that https://github.com/puppetlabs/puppet-module-gems/blob/9c488b456b3d3c1bc6fd02718ee95c102eb78211/config/dependencies.yml#L67 currently is excluding RPF v2 from being installed, so I'll fix that too.

@DavidS DavidS self-assigned this Nov 9, 2020
@DavidS
Copy link
Contributor

DavidS commented Nov 9, 2020

Results from my local tests:

# with released rspec-puppet-facts

Finished in 44 minutes 23 seconds (files took 3 minutes 20.2 seconds to load)
7803 examples, 0 failures


real	47m48.315s
user	46m6.912s
sys	1m16.671s
david@zion:~/git/puppetlabs-apache (main)$

# with modified rspec-puppet-facts
Finished in 42 minutes 47 seconds (files took 14.31 seconds to load)
7803 examples, 0 failures


real	43m5.625s
user	41m43.348s
sys	1m8.159s
david@zion:~/git/puppetlabs-apache (main)$

As you can see the file load time was reduced from 3 minutes 20 seconds down to 14 seconds. I consider that a great win (even when it doesn't make a big difference for this particular test suite).

@DavidS DavidS merged commit c2987a1 into voxpupuli:master Nov 9, 2020
DavidS added a commit to DavidS/puppet-module-gems that referenced this pull request Nov 9, 2020
Verified this version working with puppetlabs-apache unit tests. See voxpupuli/rspec-puppet-facts#122
DavidS added a commit to DavidS/puppet-module-gems that referenced this pull request Nov 9, 2020
Verified this version working with puppetlabs-apache unit tests. See voxpupuli/rspec-puppet-facts#122
@ekohl ekohl deleted the fact-memoization branch November 9, 2020 12:59
@ekohl ekohl mentioned this pull request Nov 9, 2020
@ekohl
Copy link
Member Author

ekohl commented Nov 9, 2020

Thanks for merging this. Looking forward to a release that includes this.

As you can see the file load time was reduced from 3 minutes 20 seconds down to 14 seconds. I consider that a great win (even when it doesn't make a big difference for this particular test suite).

It also helps greatly if you run something like rspec -e mytest. Note that the 14 seconds is dominated by a single on_supported_os. There's room for more improvement so I opened #123 with some of my ideas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memoizing facts
4 participants