Remove unnecessary class parameters from nginx::service

[tdevelioglu]

Class nginx::service is private, declared once and should reference
variables only from the main class.

[ekohl]

I disagree with this change. The biggest benefit of explicit class parameters is that you can write tests without including the entire module but just the class by explicitly passing in parameters. This provides huge benefits in performance and allows writing real unit tests. IMHO every class should explicitly declare its variables and only use local scope vars but I do realize it's a lot of duplication.

[binford2k]

Fwiw, it's pretty easy to mock out the variables in a spec even without passing them directly to the class. For example,

   let(:pre_condition) {
        class nginx {
           $service_restart = true,
           $service_ensure  = 'installed',
           $service_name    = 'nginx',
           $service_flags   = undef,
           $service_manage  = true,
        }
        include nginx
    }

[ekohl]

@binford2k that still includes the entire class which is very slow. Especially on big modules this really slows you down.

[tdevelioglu]

It allows setting the same variables in two places though and in a private class even. But what I really dislike is the unnecessary duplicate lookups it causes.

[bastelfreak]

I'm not really sure if we should go that path or not. @binford2k is there an official style that Puppetlabs suggests for private classes? @voxpupuli/collaborators how do you all think about this?

[jhoblitt]

I generally agree with @tdevelioglu's PR comment. I also conceded that sometimes sub-ideal patterns are a necessary concession to practical testing. However, I think that directly testing a private class is best avoided. From a behavioral standpoint, the only concerns should be the [public] interface and the resulting resources in the catalog; what goes on inside the sausage factory should be considered fairly opaque.

[wyardley]

I can see both sides of this; don't have a string feeling -- I've written and tested stuff both ways. I tend to lean towards taking them out, but it does make testing a bit trickier.

But, I think $nginx::foo is now preferred over $::nginx::foo, so if we do go this way, we should at least use that.

[ekohl]

But, I think $nginx::foo is now preferred over $::nginx::foo, so if we do go this way, we should at least use that.

IMHO we should enforce this via a linter rule and explicitly disable the old style rule.

[jhoblitt]

I've been bitten by $foo::bar being ambiguous in site modules -- has the variable resolution logic changed to make the :: implicit?

[wyardley]

I've been bitten by $foo::bar being ambiguous in site modules -- has the variable resolution logic changed to make the :: implicit?

Short answer, yes
Some discussion in voxpupuli/modulesync_config#369

[binford2k]

My preference is also to test the public interface only, except in specific edge cases. As a module developer, I should be free to change the implementation details as long as it maintains the public interface.

And for the edge cases, I'm quite ok with needing to write a precondition mock to enable the test.

[dhollinger]

@ekohl You can test the individual private classes as needed through the public interface, either through a pre_condition or by setting the values of the main class parameters within the let(:params). Sure it's slower, but it's better than making every class public just to speed up tests.

[LongLiveCHIEF]

I'm also on board with testing through the public interface. Regardless of how quick/slow it is,it's best to test the way implementors implement.

I actually like this PR. @bastelfreak and I were having some discussion about redundant parameter validation inside a private class, and this practice completely eliminates the problems behind our discussion.

👍

[bastelfreak]

I agree with @binford2k and @dhollinger here. to quote one of my teachers from university 'only test the public interface you provide to your users. (Also try to test each different failure case, not success case)'.

[ekohl]

I still see this class as a private class but I have applied this pattern because it allows unit testing. Testing the public interface is integration testing. Ideally you do both if there's complex logic in a class. The Puppet testing stack doesn't allow me to do that but this pattern is a workaround. We've started to apply this pattern in some places when our Travis tests started to take 2 hours and were getting killed.

If someone showed me how I could mock out the main class so it's just a data container then this pattern would be redundant

[bastelfreak]

@ekohl isn't that what you are looking for? #1209 (comment)

[bastelfreak]

The majority of the people seem to vote for removing the params. I will leave this open until Tuesday evening for more feedback. In my opinion we can merge this if there are no new objections.

[ekohl]

@bastelfreak if that would work, that'd be great but in the past I already tried that. Just to check I just retried it in another module:

Class 'foreman' is already defined (line: 2); cannot redefine (file: /home/ekohl/dev/puppet-foreman/spec/fixtures/modules/foreman/manifests/init.pp, line: 230)

In general I don't particularly care what other modules do and I'd be fine with either solution, but I am arguing here in favor of better testability of modules. Especially when you apply the rspec-puppet-facts pattern of testing every class with all supported operating systems you quickly explode in testing time. patterns like these help you to reduce that.

[LongLiveCHIEF]

@ekohl the nginx::service class should not be unit tested outside of the nginx module, since it should only ever be called from the nginx module. That's the very definition of a private class.

There's no need to unit test another module from your own module. You test these concerns by integration/behavioral testing of your module, combined with unit tests of your module that confirm the right public attributes are sent to the resources you've included in your modules manifests.

If you feel like there is a test that is missing for nginx::service, then you should add that unit test as a PR to this module.

[ekohl]

@LongLiveCHIEF I think you you're missing ym point. There are cases where I want to test a private class inside my own module. I want to unit test nginx::service inside this module. Sometimes you do have complex logic or want to test a combination of parameters and ignore the many other classes.

Note that when I talk about nginx::service it's just an example. This pattern can be applied to many modules.

Let me state it another way: I know of no other way than this pattern to properly unit test my own private classes.

[dhollinger]

@ekohl Yet with your example, you are now moving outside of the intended functionality of that class and module. If that class is already tested within it's own development process, it's redundant to test it again within yours.... unless you've changed the functionality or using it in an unintended way.

There is such a thing as too much testing. We test nginx::service here with Unit and Beaker testing (through the main init) which validates that the class does what it is intended to do. At that point, all your tests need to do is validate your code and use your integration/beaker tests to validate that the service resource is placed as expected. No need to re-test what's already been tested.

[juniorsysadmin]

Especially when you apply the rspec-puppet-facts pattern of testing every class with all supported operating systems you quickly explode in testing time

This is a separate unrelated problem of doing a battery of tests with low/zero value. See voxpupuli/puppet-collectd#633 for more on this.

[ekohl]

@ekohl Yet with your example, you are now moving outside of the intended functionality of that class and module. If that class is already tested within it's own development process, it's redundant to test it again within yours.... unless you've changed the functionality or using it in an unintended way.

Just to be clear, the way I've been talking about nginx::service here is as if I was a developer working on the nginx module and writing tests inside the nginx module. At no point did I think about writing anything from another module that uses nginx::service directly.

There is such a thing as too much testing. We test nginx::service here with Unit and Beaker testing (through the main init) which validates that the class does what it is intended to do. At that point, all your tests need to do is validate your code and use your integration/beaker tests to validate that the service resource is placed as expected. No need to re-test what's already been tested.

Any my point was that it can be easier to unit test a class without all the other classes.

IMHO you can't unit test a class through init, that's an integration test. I see beaker tests as smoke tests. They all have their value.

My point has been that the pattern of moving all parameters to local scope variables allows you to make a class standalone which allows unit testing. Otherwise you always have to include other classes. It removes the need to go through the full matrix of operating systems for that particular class. You can still verify the OS behavior in an integration test through init.

[LongLiveCHIEF]

my point was that it can be easier to unit test a class without all the other classes

The problem with that technique is that the class is never called by itself, so you're unit testing under conditions that can literally never happen, introducing the possibility of false positives into your tests.

I think we're ready to merge this at this point, right?

[LongLiveCHIEF]

Looks like all we're waiting on is changing $::foo to $foo.

[ekohl]

The problem with that technique is that the class is never called by itself, so you're unit testing under conditions that can literally never happen, introducing the possibility of false positives into your tests.

This is not true. Suppose you have a class with all variables that all inherit from a main class. Sometimes you just want to know if certain combinations of those variables end up with the correct result. You have simple if/else/case logic without complex inheritance, in other words a unit test. Testing it via the main class would be an integration test.

However, I guess I'm not going to convince anyone there's a gap in testing capabilities so I'll just give up. Because I'm going to need time to migrate away from Puppet I'm just going to do enough to keep my envs running but I'm not going to invest in getting infrastructure like that to run.

[LongLiveCHIEF]

@ekohl the nginx::service class isn't just a private class, it's contained within the init.pp. This means it is essentially part of that class, and is only it's own class as an organizational technique in puppet to group similar resources and treat them as one resource in the containing manifest (with pointers at the top and bottom of the contained class).

If nginx::service was a private class that wasn't contained, and was instead include'ed by multiple other classes in this module, then you are absolutely right in that it would be beneficial to test this class in isolation, and should leave these variables as class parameters.

The nature of the contain however changes everything, making nginx::service less of a "class", and more like a distinct group of resources embedded in the modules main class. That is why everyone here is insisting that it not be tested by itself. That particular class isn't a class because it's an entry point, it's a class for internal module organization and resource ordering purposes.

I realize now that this is a key distinction, and could have caused some of the frustration you've had in trying to express your point.

My apologies for not making this distinction earlier. If you want to continue to discuss the finer details of it, I'm enjoying the discussion, and would be happy to continue it in our Slack or IRC channels. Please don't let one experience with individuals factor into your decision about what tools are best for managing your infrastructure.

[ekohl]

I realize now that this is a key distinction, and could have caused some of the frustration you've had in trying to express your point.

It is one distinction, but I'd like to emphasize again that I don't care about this particular class. This is simple enough that unit testing doesn't add a lot of value. My point was about the bigger picture. A class like https://github.com/theforeman/puppet-foreman_proxy/blob/master/manifests/proxydhcp.pp which uses a parameter to extract values from facts. That is pure logic that can be tested without all the other classes included. The also needs to be updated to support the new facts.

Of course one can argue that that class is more of a profile class inside a module and you'd probably be right.

Please don't let one experience with individuals factor into your decision about what tools are best for managing your infrastructure.

Don't worry, it's been leading up to this for months. My biggest source of frustration is that puppetlabs' modules almost feel unmaintained and getting any change in takes so much effort.

[bastelfreak]

Hey everybody. I created https://tickets.puppetlabs.com/browse/MODULES-7550 to get some official feedback from Puppet.

[wyardley]

@bastelfreak FWIW, I have had some discussions with module team members about this before. IIRC, they leaned towards testing included or contained private classes via the main class, and referencing $foo::bar (for module foo) directly in the included classes.

[jorhett]

So I think the key question is what is the module's public interface?

If the module allows / documents direct calls to child classes then each of them should be tested independently. If the module only provides a single public interface via the main class, then that's how it should be tested.

A private internal class has no responsibility for preserving its interface for public use. You can choose to test its interfaces, or you can choose to test only the public interface. That's your call.

IMHO the pattern of passing all variables into a class is important and useful when that class can be called directly. It avoids a loop of the child class having to include the parent class for testing purposes. If a class is an internal, private class there is absolutely zero need to allow it to function without the parent class dependencies, and it will GREATLY simplify testing and code maintainability. I've seen a 90% reduction in test artifacts without losing a single bit of test coverage with that simplification.

IMHO the proposed change would greatly simplify testing and improve maintainability. I suggest we merge it.

I don't know that @ekohl is still paying attention here, but if so I have some thoughts that might help you understand why what you're testing doesn't provide the value you are seeking. If you're not already on the #testing channel in Puppet community, let's talk about that there. It's a good conversation to have.

[vox-pupuli-tasks]

Dear @tdevelioglu, thanks for the PR!

This is pccibot, 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

[ekohl]

In #1414 I go a step further. Since that removes almost all logic, there is no benefit to unit testing anymore.