voxpupuli / modulesync_config

configuration for our module sync
Apache License 2.0
10 stars 72 forks source link

Set default_facts in spec_helper.rb? #167

Closed jyaworski closed 8 years ago

jyaworski commented 8 years ago

I was talking with @alexjfisher and he has an interesting paradigm with rspec-puppet-facts inside of RSpec.configure.

  on_supported_os.each do |os, facts|
    puts "Setting default facts for #{os}"
    config.default_facts = facts.merge( {
      :augeasversion        => '1.2.0',
      :concat_basedir       => '/var/lib/puppet/concat',
      :domain               => 'example.com',
      :puppetmaster         => 'puppet.example.com',
      :sudoversion          => '1.8.6p3',
    })
  end

This sets the default facts. This is good for something like collectd, which I had to hard-code a bunch of basic facts inside of the tests themselves. Also, we could make this configurable. For collectd, the default fact for collectd_version can be set here in the spec helper (configurable with .sync.yml) and I can remove quite a lot of lines from my spec tests.

@bastelfreak @rnelson0

bastelfreak commented 8 years ago

:+1: for this. We already have something similar in the zabbix module.

rnelson0 commented 8 years ago

I'd be up for this if we can test it in a module branch, maybe even collectd, to ensure it works the way we expect - especially to see how overrides work.

alexjfisher commented 8 years ago

The more I look at that, the more I want to double check what I'm actually doing... For instance, am I sure I actually had more than 1 supported OS in my metadata.json???

jyaworski commented 8 years ago

@alexjfisher I'm more concerned about if rspec can store facts per-os or if it just uses the last one. We need a rspec expert. Tagging @daenney.

alexjfisher commented 8 years ago

@jyaworski Yep. Pretty sure this won't work. The each was misleading (I have an array of just one supported_os as this was for testing my 'role' module). I should really refactor...

This is the spec_helper I shared via IRC/gist. Works just fine for what I use it for, but not much use as a generic spec_helper.

require 'puppetlabs_spec_helper/module_spec_helper'
require 'rspec-puppet-facts'
include RspecPuppetFacts

if ENV['DEBUG'].to_s.downcase == 'yes'
  Puppet::Util::Log.level = :debug
  Puppet::Util::Log.newdestination(:console)
end
RSpec.configure do |c|
  c.hiera_config = 'spec/fixtures/hiera.yaml'
  on_supported_os({
    :supported_os => [
      {
        "operatingsystem" => 'OracleLinux',
        "operatingsystemrelease" => ["6"]
      }]
  }).each do |os, facts|
    puts "Setting default facts for #{os}"
    c.default_facts = facts.merge( {
      :augeasversion        => '1.2.0',
      :concat_basedir       => '/var/lib/puppet/concat',
      :domain               => 'example.com',
      :puppetmaster         => 'puppet.example.com',
      :sudoversion          => '1.8.6p3',
      :latest_uek_available => '3.8.13-118.3.2.el6uek'
    })
  end
end
bastelfreak commented 8 years ago

another solution would be to only put the facts into a dedicated file like this (from here):

def mocked_facts
  {
    concat_basedir: '/tmp',
    is_pe: false,
    selinux_config_mode: 'disabled'
  }
end

then we can built up the spec_helper,rb like this (from here):

require 'puppetlabs_spec_helper/module_spec_helper'
require 'rspec-puppet-facts'
include RspecPuppetFacts
require 'spec_helper_methods'

and we use it in the spec tests itself like this(copied from here):

  on_supported_os.each do |os, facts|
    context "on #{os} " do
      let :facts do
        facts.merge(
          mocked_facts
        )
      end
...
daenney commented 8 years ago

I'd be hesitant about having "default facts" that are applied to many things that set arbitrary facts like sudoversion etc. I prefer @bastelfreak's proposed approach. It avoids repeating yourself across tests for a module while making the loading of them explicit and easier to control.

jyaworski commented 8 years ago

Actually, that makes a lot of sense.

def mocked_facts
  {
    concat_basedir: '/tmp',
    is_pe: false,
    selinux_config_mode: 'disabled',
    puppetversion: Puppet.version,
    facterversion: Facter.version,
    ipaddress: '172.16.254.254'
    macaddress: 'AA:AA:AA:AA:AA:AA'
  }
end

There are more that I'm forgetting at the moment.

jyaworski commented 8 years ago

See #169.