The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

Bug #13595

initialize_everything_for_tests couples modules to specific Puppet versions

Added by Anonymous over 2 years ago. Updated about 2 years ago.

Status:ClosedStart date:04/02/2012
Priority:UrgentDue date:04/11/2012
Assignee:Chris Price% Done:

0%

Category:testing
Target version:2.7.14
Affected Puppet version: Branch:see blocker tickets
Keywords:

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com

This issue is currently not available for export. If you are experiencing the issue described below, please file a new ticket in JIRA. Once a new ticket has been created, please add a link to it that points back to this Redmine ticket.


Description

Ran into this while attempting to run spec tests on the puppetlabs-ntp module from Git:

Failure/Error: Unable to find matching line from backtrace
NoMethodError:
undefined method `initialize_everything_for_tests' for #<Puppet::Util::Settings:0xb71fb30c>

It turns out that :initialize_everything_for_tests was recently added to Puppet itself:

https://github.com/puppetlabs/puppet/commit/56c55d54474d97958f0f21e715237cf3f2117668

There are two problems here:

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.


Related issues

Related to Puppet - Bug #13439: specs API incompatibility between 2.7.x and master Closed 03/27/2012
Related to Puppet - Feature #13509: Establish or solidify pattern for testing modules Accepted 03/29/2012
Blocked by Puppet - Refactor #13690: Update puppet 2.7.x for spec_helper compatibility with ex... Closed 04/09/2012
Blocked by Puppet - Refactor #13691: Update puppet master for spec_helper compatibility with e... Closed 04/09/2012
Blocked by Puppet - Refactor #13692: Update puppetlabs_spec_helper to use new puppet core spec... Closed 04/09/2012
Blocked by Standard Library - Refactor #13693: Modify stdlib spec_helper to use puppetlabs_spec_helper Merged - Pending Release 04/09/2012
Blocked by Puppet - Feature #13695: documentation for spec compatibility between external pro... Closed 04/09/2012

History

#1 Updated by Kelsey Hightower over 2 years ago

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.

#2 Updated by Jeff McCune over 2 years ago

What if we add all puppet specific initialization logic to the spec helper that lives outside of puppet at https://github.com/puppetlabs/puppetlabs_spec_helper?

#3 Updated by Jeff McCune over 2 years ago

  • Project changed from Puppet to Standard Library

#4 Updated by Jeff McCune over 2 years ago

  • Project changed from Standard Library to Puppet

#5 Updated by Chris Price over 2 years ago

That sounds promising—are modules et al already using that project?

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…

#6 Updated by Chris Price over 2 years ago

  • Due date set to 04/11/2012
  • Category set to testing
  • Status changed from Unreviewed to Accepted
  • Assignee set to Chris Price
  • Priority changed from Normal to Urgent
  • Target version set to 2.7.x

So, here is a straw-man proposal… feedback / suggestions / corrections much appreciated:

  1. Revert my changes to various spec_helper classes (puppet 2.7.x, puppet master, stdlib, grayskull, and Eric’s change).
  2. 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.
  3. 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”.
  4. 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:

  1. 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.
  2. 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.
  3. 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.

#7 Updated by Chris Price over 2 years ago

p.s., in order to prevent the current code from shipping with the next puppet release, we will need to make a decision on this in the next week or so.

#8 Updated by Kelsey Hightower over 2 years ago

+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.

#9 Updated by Jeff McCune over 2 years ago

Chris Price wrote:

So, here is a straw-man proposal… feedback / suggestions / corrections much appreciated:

  1. Revert my changes to various spec_helper classes (puppet 2.7.x, puppet master, stdlib, grayskull, and Eric’s change).
  2. 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.
  3. 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”.
  4. 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:

  1. 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.”
  2. 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.

#10 Updated by Chris Price over 2 years ago

Jeff McCune wrote:

  1. 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.

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.

  1. Some rather prevalent documentation or communication on “The Standard Way” to initialize puppet for testing will be invaluable for the community.

Excellent, agreed and thanks for the heads up on both of the above.

#11 Updated by Chris Price over 2 years ago

I intend to go ahead and start working on this today, so if anyone has any additional input please let me know.

#12 Updated by Chris Price over 2 years ago

I had a couple of requests to create individual tickets for the various steps of this process… have done so now, and they are all linked.

#13 Updated by Chris Price over 2 years ago

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.

#14 Updated by Chris Price over 2 years ago

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…

#15 Updated by Chris Price over 2 years ago

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.

#16 Updated by Chris Price over 2 years ago

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?

#17 Updated by Anonymous over 2 years ago

Now that I’ve waited an ungodly time to comment on this, +1 on the plan, presuming the Facter issues can get worked out.

-Eric

#18 Updated by Chris Price over 2 years ago

So, I have five pull requests in that I think cover this… they are:

Puppet 2.7.x: https://github.com/puppetlabs/puppet/pull/647 (This is the most important one)

Puppet master: https://github.com/puppetlabs/puppet/pull/653

Puppetlabs_spec_helper: https://github.com/puppetlabs/puppetlabs_spec_helper/pull/1

stdlib: https://github.com/puppetlabs/puppetlabs-stdlib/pull/61

grayskull: https://github.com/puppetlabs/puppetdb/pull/119

I haven’t written the docs yet, would prefer to wait on feedback before I do that.

I plan to do a bit more testing with some clean working copies this morning and will post results here, but I think that these should be just about ready to go.

#19 Updated by Chris Price over 2 years ago

Local test results:

  • puppet 2.7.x branch specs pass
  • puppet master specs pass

assuming puppetlabs_spec_helper project is in RUBYLIB:

  • stdlib branch specs pass against 2.6.5, 2.6.x, 2.7.0, 2.7.5, 2.7.10, 2.7.12, 2.7.x, 2.7.x pull req branch, and master pull req branch
  • grayskull specs pass against 2.7.10, 2.7.12, 2.7.x, 2.7.x pull req branch, and master pull req branch.

I’m going to go into the pull requests and remove the “FOR REVIEW ONLY” labels—would like to get these merged today if possible. Happy to help deal with the ensuing jenkins chaos.

#20 Updated by Chris Price over 2 years ago

  • Status changed from Accepted to In Topic Branch Pending Review

#21 Updated by Chris Price over 2 years ago

  • Branch set to see blocker tickets

#22 Updated by Chris Price over 2 years ago

It seems like this set of changes is working OK in CI now, and I think this ticket can be closed as soon as the 2.7.14 rc is cut.

#23 Updated by Chris Price over 2 years ago

  • Target version changed from 2.7.x to 2.7.14

#24 Updated by Chris Price over 2 years ago

  • Status changed from In Topic Branch Pending Review to Merged - Pending Release

#25 Updated by Moses Mendoza about 2 years ago

  • Status changed from Merged - Pending Release to Closed

Released in 2.7.14rc1.

Also available in: Atom PDF