Skip to content
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

Tests for prometheus::daemon #87

Merged
merged 1 commit into from
Nov 7, 2017
Merged

Conversation

sathieu
Copy link

@sathieu sathieu commented Oct 24, 2017

No description provided.

@@ -0,0 +1,161 @@
#! /bin/sh
Copy link
Contributor

Choose a reason for hiding this comment

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

To me, having the entire body of both init scripts seems a bit like overkill, maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

whats the point of checking the complete content of the init scripts/unit file anyway?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's what I mean.

Copy link
Author

Choose a reason for hiding this comment

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

the point is that it is hard to ensure correct file syntax with all those new lines and backslash. But your are the reviewer, tell me what you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sathieu i don't see how this check helps. if you change the files shipped in the module you need to change the files shipped in the fixtures. so basically you are duplicating content without any benefit added

Copy link
Author

Choose a reason for hiding this comment

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

I generally agree, but this kind of rspec helped for https://github.com/voxpupuli/puppet-prometheus/pull/48/files (by ensuring that those <%= @daemon_flags.join(" \\\n ") %> or like were correct)

Copy link
Contributor

Choose a reason for hiding this comment

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

how about testing it this way

          if ['debian-7-x86_64'].include?(os)
            # init_style = 'debian'

            it {
              is_expected.to contain_file('/etc/init.d/smurf_exporter').with(
                'mode'    => '0555',
                'owner'   => 'root',
                'group'   => 'root',
              )
              should contain_file('/etc/init.d/smurf_exporter').with_content(/DAEMON_ARGS=''\n/)
            }

also prometheus::daemon doesn't actually set $daemon_flags so the join in this context always evaluates to ''

require 'spec_helper'

describe 'prometheus::daemon' do
let :title do
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be :title or :params and then setting :title in params?

Copy link
Author

Choose a reason for hiding this comment

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

this is usually :title

@sathieu
Copy link
Author

sathieu commented Nov 7, 2017

Here is the updated PR. No more using fixtures.

@wyardley wyardley self-requested a review November 7, 2017 22:09
Copy link
Contributor

@wyardley wyardley left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me.

@wyardley wyardley merged commit a2bb6c1 into voxpupuli:master Nov 7, 2017
Rovanion pushed a commit to Rovanion/puppet-prometheus that referenced this pull request May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants