-
Notifications
You must be signed in to change notification settings - Fork 183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Modernize the acceptance tests #438
Conversation
91681c2
to
096f841
Compare
RSpec::Matchers.define_negated_matcher :execute_without_warning, :execute_with_warning | ||
|
||
# systcl settings are untestable in docker | ||
describe 'redis::administration', unless: default['hypervisor'] =~ %r{docker} do | ||
it 'runs successfully' do | ||
pp = <<-EOS | ||
include redis | ||
include redis::administration | ||
EOS | ||
def execute_with_warning | ||
have_attributes(stderr: %r{WARNING}) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This here is to combine things with .and
since you can't use .not_to().and()
.
096f841
to
3471e1c
Compare
3471e1c
to
e3ff47e
Compare
Updated to top the Debian backport acceptance test. See that commit for details. |
e3ff47e
to
eb48914
Compare
I had one copy-paste error where |
it { is_expected.not_to be_running } | ||
end | ||
specify { expect(package(redis_name)).not_to be_installed } | ||
specify { expect(service(redis_name)).not_to be_enabled.and be_running } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line confuses me. As mentioned in the comment above - isn't not_to ...and
disallowed in RSpec in some way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, here it is:
`expect(...).not_to matcher.and matcher` is not supported, since it creates a bit of an ambiguity. Instead, define negated versions of whatever matchers you wish to negate with `RSpec::Matchers.define_negated_matcher` and use `expect(...).to matcher.and matcher`.
I'd suggest either matching against a negated predicate if it exists (disabled?
and stopped?
) with a regular .to
runner:
specify { expect(service(redis_name)).to be_disabled.and be_stopped }
If those predicates are not available on what service
returns, define negated matchers for be_running
and be_enabled
.
Or just plain split this expectation in two:
specify do
expect(service(redis_name)).not_to be_enabled
expect(service(redis_name)).not_to be_running
end
Whatever you like the most.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two different tests is much easier to read and maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose you meant two spec examples.
Sure, I agree, unless expensive setup is involved, which I've heard, is sometimes the case with serverspec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right that this line is vague and confusing.
It is very common to always want it both enabled and running or both disabled and stopped. I've been wondering if it makes sense to have some short hand for it, but I struggled to find a good name for it.
Looking at https://serverspec.org/resource_types.html#service there is no be_disabled
nor be_stopped
. I'll think a bit more about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I went with the recommendation to split it in two statements. That was the easiest.
For example, not_to be_running
is clear. But is to be_stopped
the same? to be_stopped
could imply it should be present but stopped while not_to be_running
doesn't care whether the service is present or not. Splitting it in two avoids these issues.
ef1500d
to
79f31d4
Compare
79f31d4
to
302e109
Compare
I think this should be good to merge. I'm leaning to dropping Archlinux from the metadata for now since the tests obviously fail. Also #439 suggests it's entirely broken at the moment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with dropping Arch Linux from the metadata.json for now, or merging this as is.
Dear @ekohl, thanks for the PR! This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
2 year ping @ekohl :) do you want to rebase this to the current master? |
Archlinux doesn't have an AIO installation so the puppet_gem provider doesn't work. Using the regular gem provider should work. The aio_agent_version fact is used to determine whether it's an AIO install.
40e6d9d
to
9f7c674
Compare
Rebased to resolve merge conflicts. I didn't run it locally yet, so it may have broken something in the rebase. |
9f7c674
to
18e516c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont know if we want to fix the legacy facts in this PR or do all legacy stuff in a second one. Additionally we should look at why the Puppet 7 on Centos 8 test times out.
Otherwise this looks good :)
It's common for the CentOS 8 tests to time out. I suspect pulling from quay.io may cause problems. |
18e516c
to
5047ec5
Compare
Updated to drop legacy facts in the test suite. |
This takes the lessons from rubocop/rubocop-rspec#1231 and applies them here.