1) We have no current documentation or method of knowing which version of Puppet is required for a particular module. We tie to specific module versions but not to Puppet itself – anyone trying to test the latest puppetlabs-ntp against the most recently cut version of Puppet or PE will fail.

2) We should be iterating on modules much more quickly than we are on core, but this means that modules need to be slightly more backward-compatible – we can’t rely on the “latest and greatest” stuff in master or 2.7.x if modules are going to be updated frequently. It might make more sense to move this type of function out to stdlib.

Commenting out the lines calling this function in the spec_helper.rb make the problem go away, but I suspect we’re going to run into a lot more of these if we don’t figure out an approach to it.

History

I think it would be awesome to have something like rspec-modules, where we can have a central place for module testing stuff. I don’t have details right now, but I think this could be the right thing to do.

I’ve chatted with a few folks about this issue and the more I think about it, the more I am convinced that it would be nice to have a longer-term solution in place before the current approach gets released. So, either a separate project like the one Jeff references, or:

I could just add a class like “Puppet::SpecHelper” into puppet’s main “lib” area, and give it two simple setup/teardown methods… this would make it more general / global than the current approach which is tightly coupled to P::U::Settings.

Actually, perhaps this should be done in combination with Jeff’s suggestion of having modules rely on puppetspec_helper…

Create a new class as part of puppet core: Puppet::Util::Test::Setup, or something to that affect. (Name suggestions particularly welcome.) Give it two methods: setup() and teardown(), which would be considered the public API for interacting with puppet during tests. This would be defined in 2.7.x and master; implementation details would be irrelevant to the outside world.

Use the puppetspec_helper project that Jeff mentioned to wrap this: it would need to have a single hack for the time being, which would basically be “if the class Puppet::Util::Test::Setup exists, call its methods, otherwise fallback to the old behavior”.

Fix spec helpers in various external projects (stdlib, grayskull, et al.) to use puppetspec_helper rather than attempting to interact with puppet’s setup/teardown directly; this prevents external projects from having to include a copy of the hack described in #3.

The only downside that I see with this is that the jenkins jobs would need to be modified to pull down puppetspec_helper as part of their builds… and that seems palatable to me. Am I missing anything? Anyone else have any other concerns / suggestions?

Here are a few facts that the proposal above is based on:

We can’t entirely revert the changes that I made to the spec_helpers and go back to the “old way”, because the “old way” is not compatible with master—and can’t be. There were some changes that were necessary to complete some roadmap goals with Telly that simply aren’t compatible with the way the old spec_helpers were working.

It seems logical to me that external projects should not be responsible for managing puppet’s state during test setup / teardown, so introducing new API for this seems necessary, and achieving 100% backward compatibility without modifying spec_helpers in external projects is unavoidable.

In the long run, separation of concerns between a) implementation details of puppet state management for testing, b) requests for this state management by external projects, and c) implementation details of bridging the gap between “a” and “b” seems desirable.

+1 This something we talked about before and having the API will make it much easier for external testing tools. I guess we need to make sure we keep this API going forward with good docs on how/why to use it.

Create a new class as part of puppet core: Puppet::Util::Test::Setup, or something to that affect. (Name suggestions particularly welcome.) Give it two methods: setup() and teardown(), which would be considered the public API for interacting with puppet during tests. This would be defined in 2.7.x and master; implementation details would be irrelevant to the outside world.

Use the puppetspec_helper project that Jeff mentioned to wrap this: it would need to have a single hack for the time being, which would basically be “if the class Puppet::Util::Test::Setup exists, call its methods, otherwise fallback to the old behavior”.

Fix spec helpers in various external projects (stdlib, grayskull, et al.) to use puppetspec_helper rather than attempting to interact with puppet’s setup/teardown directly; this prevents external projects from having to include a copy of the hack described in #3.

The only downside that I see with this is that the jenkins jobs would need to be modified to pull down puppetspec_helper as part of their builds… and that seems palatable to me. Am I missing anything? Anyone else have any other concerns / suggestions?

This seems totally reasonable and fine by me.

The only two additional pieces of information I’d like to add are:

It’s always bugged the crap out of me that irb -r puppet gives you something only semi-functional and does a LOT of initialization of the framework configuration for you. This irritation is dated about 18 to 24 months ago though. I believe dpittman may have improved this substantially since then so this may be a moot point. I mention this annoyance of mine because initializing the framework for testing is very similar to initializing the framework for other uses like interactive shells or other applications. It would be great if you keep an eye toward better ways to separate out loading the framework and initializing the framework to make the framework easier to work with. There are no action items on this point, it’s just something I’d ask that you keep in mind since you’ll be in this part of the code. The litmus test I use to determine if it’s easy to work with is something like, “What are the number of steps after irb -r puppet I need to make to get Puppet into the state I want it in for my use case? The lower the number, the better.”

Some rather prevalent documentation or communication on “The Standard Way” to initialize puppet for testing will be invaluable for the community. Putting it in the README_DEVELOPER will be a great first step, we can take it from there and bake it into puppet module generate or what not. I mention this not because I don’t think you wouldn’t already do this but rather because I just didn’t see it mentioned in this straw man.

First pull request for this (ticket #13690, the 2.7.x bits) is up for review: https://github.com/puppetlabs/puppet/pull/647 . The rest of them should be pretty simple but will be based off of this one, so if anyone has time to take a quick peek and make sure that this API looks sane before I start working on the other ones, that would be swell.

Good times. I just realized that the existing puppetlabs_spec_helper was created by Ken about 4 months ago, to reduce spec utility code duplication between facter specs and puppet specs. However, facter is one of our external projects that does not have a dependency on puppet core; thus, introducing the puppet spec state management code into puppetlabs_spec_helper.rb would break facter tests.

At the moment, I’m leaning towards adding a second file to the same repo… probably called “puppet_spec_helper.rb” or “puppet_core_spec_helper.rb”. This way, facter will still be able to have a dependency on the project (puppetlabs_spec_helper), but it can simply choose not to ‘require’ the puppet-core-specific spec_helper, and thus avoid having a dependency on puppet core.

I’m not thrilled with this idea / approach, but it seems more sane than introducing yet another repository to contain this new spec helper…

just talked this over with Jeff. Planning on proceeding with the plan described in the previous comment, but the new file will be named “puppetlabs_puppet_spec_helper.rb”. The idea is that in the future there might also be, e.g., “puppetlabs_facter_spec_helper.rb”, “puppetlabs_mcollective_spec_helper.rb”, “puppetlabs_cloud_provisioner_spec_helper.rb”, etc.

Ken—it looks like the current facter just has all of the spec_helper stuff copied in to it directly? e.g. you are not actually referencing the shared code in the separate puppetlabs_spec_helper project?