Refactored spec tests

[ghost]

Broke out all config files to fixtures, tried to minimize code duplication.

[ghoneycutt]

I think the issue is with r.push Packages to build the require string. Here's how I solved this before

https://github.com/ghoneycutt/puppet-module-ntp/blob/master/spec/classes/init_spec.rb#L252-288

[ghost]

@ghoneycutt I wanted to keep packages and requirements as arrays as long as possible, to prepare for the vas tests in #102, I hope that's ok.

[ghost]

@ghoneycutt I added 2 commits that are not part of the spec test, but are problems I found when writing them. Not sure if this should be separate pull requests.

[ghoneycutt]

Separate PR's are not needed, though putting them in a separate commit is nice to make it obvious what is a refactor and what is new. Since you are adding missing tests instead of functionality to code, this is easy. If you were adding functionality to the code, then that should always be in a separate PR from a bugfix or refactor.

[ghoneycutt]

Why change the resource title? This could potentially be construed as a breaking change if someone tightly coupled their module to ours and they have a relationship to this resource.

[ghost]

That's part of the "Standardize naming" commit. It would remove some special cases from the spec tests, but it not needed.

[ghoneycutt]

I guess I don't understand what reversing the name buys us. Does it make spec tests easier or something?

[ghost]

Yes, it removes the logic that has to handle that file as a special case. See https://github.com/emahags/puppet-module-pam/commit/b0b56684c24e2e891e2bb50db23a00dac42956f0

[ghost]

@ghoneycutt I have rebased the pull request without the 2 changes to noninteractive_session and suse 9 package requirements, I'll do them as separate PRs. Is there something you see that needs to be changed or added?

[ghoneycutt]

Hi @Phil-Friderici

If you have the time, I would really appreciate an extra set of eyes on this PR to ensure that no tests were accidentally dropped.

[Phil-Friderici]

@ghoneycutt I'll have a look on it
@emahags impressive work !

[Phil-Friderici]

@emahags first minimal finding.
Better use should contain_file().without_content here:
https://github.com/emahags/puppet-module-pam/blob/dde9a6be5130ffa18859af42ed946acdf4f7f8f7/spec/classes/init_spec.rb#L372
https://github.com/emahags/puppet-module-pam/blob/dde9a6be5130ffa18859af42ed946acdf4f7f8f7/spec/classes/init_spec.rb#L397

[Phil-Friderici]

@ghoneycutt There was a double check (in my eyes) that was removed. Should be ok because the file is tested as fixture that does not contain the line in question.
https://github.com/ghoneycutt/puppet-module-pam/blob/v2.15.0/spec/classes/init_spec.rb#L384-L385

[Phil-Friderici]

@ghoneycutt @emahags did check el5 today, think that others get faster now. Continue tomorrow (hopefully)

[ghoneycutt]

Thanks @Phil-Friderici !

I did functional tests of EL6 and 7 and no changes, as expected.

[ghost]

@Phil-Friderici I updated lines 372 and 397 with your recommendation. Had to filter out solaris though as they don't use those files.

[Phil-Friderici]

@emahags I don't see the checks for symlinks in the output anymore, even though I can see the code. Maybe the problem is in this line: https://github.com/emahags/puppet-module-pam/blob/spec/spec/classes/init_spec.rb#L302

[ghost]

@Phil-Friderici Good find, I forgot to change that name.

[Phil-Friderici]

@emahags tests for :vas_major_version => '4' are lost

[ghost]

@Phil-Friderici That should be checked at https://github.com/emahags/puppet-module-pam/blob/spec/spec/classes/init_spec.rb#L262. The params is enabled here: https://github.com/emahags/puppet-module-pam/blob/spec/spec/classes/init_spec.rb#L270-L272. vas_major_version was never set to 4 in the old spec tests.

[Phil-Friderici]

@emahags it wasn't needed to be set explizit to '4' [1] because the module default for it is '4' [2]

;)

[1] https://github.com/emahags/puppet-module-pam/blob/master/spec/classes/init_spec.rb#L1878-L1906
[2] https://github.com/emahags/puppet-module-pam/blob/master/manifests/init.pp#L45

[ghost]

@Phil-Friderici Did you want me to do something about this and if so what?

[Phil-Friderici]

@emahags I am just looking for completness at the moment. If there is no difference between 3 and 4 there is no need to test it. But there were specific test for 3 and 4 in the old spec tests.
Decide yourself if you think they are needed or obsolete.

[Phil-Friderici]

@emahags I can go into the details when finished with completeness checks.

[ghost]

@Phil-Friderici Unless something in the test logic isn't working, I do have tests for version 3 and the default version (4). I guess I could change my context to make it more clear that the default vas version is used.

[Phil-Friderici]

@emahags hahahaha... yes I agree too now 👍
Your new tests are done in the same way like the old ones. Making the context aware of that would help people like me ;)

[Phil-Friderici]

@ghoneycutt:
what I did:

• verified that all checks are still existing by comparing the output of old and new style rspec testings

what I didn't:

• did not verify the content of fixture files compared to the old style

conclusion:
@emahags spec testings are a big step forward ! 👍
Some stuff got fixed, some missing tests were added (dependency checks)

I would merge it

@emahags Thanks a lot for this hard work !

[ghoneycutt]

@emahags @Phil-Friderici

Amazing work! Can't believe that init_spec.rb is 80% smaller. This is really going to help people add features with the simplified testing framework.