Refactor unit tests to iterate over all supported OS's

[LongLiveCHIEF]

I was preparing to make a merge request for a new feature, and found that I couldn't really add a spec to it for both deb and rpm without duplicating about 20 LOC in the spec/classes/init_spec.rb file.

I used one of the more useful features of the rspec-puppet-facts gem, to beef up the quality of the tests, and refactored the design to allow new features to be added while typically only requiring 4 new lines of code to test the feature across all supported operating systems.

The overall goal of this PR is to allow us to more easily add new specs by increasing the flexibility of the testing utilities, while simultaneously increasing the likelihood we'll catch a bug for a supported os before it's tagged to a release.

This PR is quite large, so here's a few bullets to help navigate it:

• I reduced the Lines of Code by nearly 30%
• increased coverage (67% -> 72%)
• increased number of tests executed (54 -> 319)
• improved code quality by running each test against all supported operating systems. (most tests previously only ran against debian)
• instead of declaring a fact hash with alternative OS/architecture for a context, we can now just use a simple evaluation (if/unless/case) on an individual fact to do something like testing apt vs yum. (see code for this example)
• changed the unsupported test:

- it { expect { is_expected.to contain_package('gitlab') }.to raise_error(Puppet::Error, /OS family Solaris not supported/) }
+ it { is_expected.to compile.and_raise_error(/is not supported/) }

[LongLiveCHIEF]

For some reason, every time I run the tests on any of my local machines, everything passes, but each run is for some reason failing on travis.

Finished in 27.57 seconds (files took 1.79 seconds to load)
319 examples, 0 failures

Total resources:   29
Touched resources: 21
Resource coverage: 72.41%
Untouched resources:

  Apt::Pin[docker]
  Docker::Image[ubuntu_trusty]
  Exec[Register_runner_test_runner]
  Package[apt-transport-https]
  Package[cgroupfs-mount]
  Package[device-mapper]
  Package[xz-utils]
  Yumrepo[runner_gitlab-ci-multi-runner-source]
The 'metadata' task is deprecated. Please use 'metadata_lint' instead.
metadata-json-lint metadata.json

It has to do with the :gitlab_systemd fact, and how I'm using it, so i'm going to have to figure that out. It's likely related to rspec-puppet-facts/41, and we may have to settle for defining this fact the old fashioned way in specs for the time being.

[LongLiveCHIEF]

actually, I realize now that this is a result of including RspecPuppetFacts in the spec_helper. I've run into this before, where compiler facts aren't found, and have to be explicitly defined.

  Failure/Error: it { should_not contain_package('gitlab-ce') }
     Puppet::Error:
       Undefined variable "::gitlab_systemd"; Undefined variable "gitlab_systemd" at /Users/chief/code/puppet-gitlab/spec/fixtures/modules/gitlab/manifests/params.pp:35 on node chief-mbp.local

I think we may need to add facter to the gemfile.

[LongLiveCHIEF]

I wound up removing the service_provider and service_initd related params.

provider rarely needs to be specified with the puppet resource
type, as puppet will reliably select the correct provider for the node.

Additionally, by passing in , start, stop , etc... puppet will reliably control the gitlab-runsvdir service using /usr/bin/gitlab-ctl commands

[LongLiveCHIEF]

At the moment, this is failing due to the usage of $::os in the install.pp manifests. This evaluation is used to determine the releasever to plug into the yumrepo.

The good news is that this should have always been failing, (since STRICT_VARIABLES is set to yes), but that particular block of code was never evaluated in tests since we were using debian as the $::osfamily for any tests that would have touched that case statement.

In order to fix this, I have two options:

• use $facts['os']['release']['major'] instead of the top-scope variable
• remove the $::os evaluation entirely, and instead use the yum variable releasever just like we're already using basearch inside the url itself.

Could one of the project owners/maintainers let me know your thoughts on this one? @tobru ?

[LongLiveCHIEF]

I decided to just use the native yum releasever. If you want to merge this but want me to take another approach, just let me know.

[tobru]

Thanks @LongLiveCHIEF for your work. It will take some time to review, I'll come back to it later.

[LongLiveCHIEF]

@tobru not a problem. I realize this is a MR of a large magnitude.

One thing this would affect, is the limitations mentioned in the README.

At the moment the module is not yet tested under RPM based operating systems.

We would be able to eliminate this statement with this Pull Request, since it adds tests for RPM, yum, and RHEL family OS's with equal coverage as debian based OS's.

But in theory it should work as all the preparations are there.

There were actually some bugs that I found and fixed, that would have caused this module to fail previously on systems in the RedHat family.

If you would like to approve this MR, and want me to update these two statements, please let me know.

I'd also like to become a contributor. I have some pretty great enhancements planned. For example: Feature-allow-package-repo-configuration

[op-ct]

Thanks, @LongLiveCHIEF!

There were actually some bugs that I found and fixed, that would have caused this module to fail previously on systems in the RedHat family.

Would you mind pointing out some of the bugs you found (and the conditions are required for them to occur)? I've been running new acceptance tests on EL7 (for release 1.13.3), and so far they haven't failed for any of the scenarios I've tried. I'm curious if I've missed a (broken) critical path somewhere, but I didn't find any in the commit diffs.

[LongLiveCHIEF]

It would have depended on your version and configuration of puppet. But here you go: 1192cc6#diff-a276806bea2dfcf1f57c33aee19c7309L53

The reason for this is what I was talking about above

[LongLiveCHIEF]

@tobru any updates? I have some other work ready to go, but I'd like to see this final review decision before I submit any further merges.

Thanks!

[tobru]

@LongLiveCHIEF Thanks a lot for your work, I think this is fine and I've merged your changes. A release will most probably bump the major version number as removing parameters is a change of the interface.

[mhyzon]

I think there is a lingering issue with this merge request. @LongLiveCHIEF you removed the references to the ::gitlab_systemd fact from init.pp, params.pp, and service.pp. But the Facter code still exists in lib/facter/gitlab_systemd.rb, so that is still getting executed on client systems. I think there is still a reference in spec/classes/init_spec.rb as well.

Is the right thing to do here just remove the Facter file and the line in the init_spec.rb file?

[LongLiveCHIEF]

Yeah, probably. I'll take a closer look tomorrow morning and provide an update.