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

Replace Puppet.initialize_settings with Puppet::Test::TestHelper #60

Merged
merged 1 commit into from
Nov 28, 2016

Conversation

domcleal
Copy link
Contributor

TestHelper offers a public API for (re)initialising Puppet reliably,
which is preferable to using initialize_settings. It also avoids
conflicts when multiple libraries call the same method.

TestHelper offers a public API for (re)initialising Puppet reliably,
which is preferable to using initialize_settings. It also avoids
conflicts when multiple libraries call the same method.
@rjw1
Copy link
Member

rjw1 commented Nov 25, 2016

Thanks for the PR one of us will hopefully look at this soon.

@rjw1
Copy link
Member

rjw1 commented Nov 25, 2016

I think this is good to merge. 👍
In my testing it adds 20% to our runtime. I think this is acceptable cost given it's the right way to do it.

@DavidS do you have any opinions about this change?

@DavidS
Copy link
Contributor

DavidS commented Nov 25, 2016

@rjw1 it does clean out all the caches, so the impact on runtime is not surprising. Given that @domcleal has found this, I do expect he's run into one of those nasty edge cases where one test had poisoned the well for another one, in which case there is no good way around it.

@domcleal
Copy link
Contributor Author

Luckily it wasn't so much a test, but I was using puppet-syntax together with the kafo_parsers gem - both used initialize_settings, and Puppet threw an error because it had already been initialised by the other gem.

Though puppet-syntax has a guard to check if Puppet's already initialised, kafo_parsers didn't, so using the proper setup/cleanup mechanism in both gems seemed the best way to fix compatibility issues permanently.

@deanwilson
Copy link
Contributor

The code is good and the explanation makes sense so I'm going to merge it. @rjw1 we should add a ticket to get a new release issued.

@deanwilson deanwilson merged commit c6fe5c8 into voxpupuli:master Nov 28, 2016
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.

None yet

4 participants