Skip to content

Conversation

@ekohl
Copy link
Collaborator

@ekohl ekohl commented Jan 5, 2021

Rather than checking the notify/require properties, it uses the matchers that check the dependency graph. That means it can also check transitive requires/notifies.

It also uses compile and compile.and_raise_error as a shorter way to check for errors (or the absense of them).

This is just a tiny improvement. The specs need a major update since a lot of the examples are using hardcoded facts of EOL OSes.

@ekohl ekohl requested a review from a team as a code owner January 5, 2021 19:15
@david22swan
Copy link
Member

@ekohl
There where some minor Rubocop errors in your code and so I pushed up a quick fix for them in order to save time.
Apologies for any offense


it 'raises an error' do
expect { expect(subject).to contain_apache__mod('php5') }.to raise_error Puppet::Error, %r{mpm_module => 'prefork' or mpm_module => 'itk'}
is_expected.to compile.and_raise_error(Puppet::Error, %r{mpm_module => 'prefork' or mpm_module => 'itk'})
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self:

Suggested change
is_expected.to compile.and_raise_error(Puppet::Error, %r{mpm_module => 'prefork' or mpm_module => 'itk'})
is_expected.to compile.and_raise_error(%r{mpm_module => 'prefork' or mpm_module => 'itk'})

@ekohl
Copy link
Collaborator Author

ekohl commented Jan 25, 2021

There where some minor Rubocop errors in your code and so I pushed up a quick fix for them in order to save time.
Apologies for any offense

That's great. If it gets things merged quicker I'm happy to see these changes.

@codecov-io
Copy link

Codecov Report

Merging #2112 (5d0bcd5) into main (0c19106) will decrease coverage by 1.04%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2112      +/-   ##
==========================================
- Coverage   57.40%   56.36%   -1.05%     
==========================================
  Files          12       12              
  Lines         216      220       +4     
==========================================
  Hits          124      124              
- Misses         92       96       +4     
Impacted Files Coverage Δ
lib/puppet/functions/apache/bool2httpd.rb 0.00% <0.00%> (ø)
lib/puppet/type/a2mod.rb 100.00% <0.00%> (ø)
lib/facter/apache_version.rb 55.00% <0.00%> (ø)
lib/puppet/provider/a2mod.rb 31.57% <0.00%> (ø)
lib/puppet/functions/bool2httpd.rb 0.00% <0.00%> (ø)
lib/puppet/provider/a2mod/a2mod.rb 61.11% <0.00%> (ø)
lib/puppet/provider/a2mod/gentoo.rb 93.84% <0.00%> (ø)
lib/puppet/provider/a2mod/modfix.rb 75.00% <0.00%> (ø)
lib/puppet/provider/a2mod/redhat.rb 64.28% <0.00%> (ø)
lib/puppet/functions/apache/pw_hash.rb 0.00% <0.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c78e73f...5d0bcd5. Read the comment docs.

@david22swan
Copy link
Member

@ekohl
Your changes have got a few failures that need to be looked at.
But overall things look good.

ekohl and others added 2 commits January 25, 2021 14:41
Rather than checking the notify/require properties, it uses the matchers
that check the dependency graph. That means it can also check transitive
requires/notifies.

It also uses compile and compile.and_raise_error as a shorter way to
check for errors (or the absense of them).
@ekohl
Copy link
Collaborator Author

ekohl commented Jan 25, 2021

There are acceptance test failures but I fail to see how they can be caused by this.

@david22swan
Copy link
Member

@ekohl After a few rekicks everything looks green so I feel happy to merge.
Thanks for putting in the work :)

@david22swan david22swan merged commit d0dcffb into puppetlabs:main Jan 27, 2021
@ekohl ekohl deleted the correct-rspec branch February 6, 2021 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants