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

Requires and autorequires do not seem to be evaluated #628

Closed
glennsarti opened this issue Dec 13, 2017 · 3 comments
Closed

Requires and autorequires do not seem to be evaluated #628

glennsarti opened this issue Dec 13, 2017 · 3 comments

Comments

@glennsarti
Copy link

I was writing some tests for the puppetlabs-registry module and I found I could not test that the autorequires was working. I have created a minimal repro case at https://github.com/glennsarti/rspec_issue

For the sake of the issue I'll copy the pertinent bits here:

Given a test manifest of (https://github.com/glennsarti/rspec_issue/blob/master/spec/fixtures/testmodule/manifests/init.pp)

class testmodule {

  notify { 'notify1': }

  notify { 'notify2':
    require => Notify['notify1']
  }

  dummy { 'Test':
    path => 'test',
  }
}

Note - Dummy is a custom type created at https://github.com/glennsarti/rspec_issue/blob/master/lib/puppet/type/dummy.rb which is used for debugging.

Given a spec file of:

describe 'testmodule' do

  # ------------------------------------------- Just using notify resources

  # This expectation succeeds
  it { is_expected.to compile }

  # This expectation succeeds
  it { is_expected.to contain_notify('notify1') }

  # This expectation succeeds
  it { is_expected.to contain_notify('notify2') }

  # This expectation fails!
  it { is_expected.to contain_notify('notify2').that_requires(["Notify['notify1']"]) }

  # Even with an explicit cataloge compile, this expectation fails!
  it {
    is_expected.to compile

    is_expected.to contain_notify('notify2').that_requires(["Notify['notify1']"])
  }
  it {
    is_expected.to compile.with_all_deps

    is_expected.to contain_notify('notify2').that_requires(["Notify['notify1']"])
  }

  # ------------------------------------------- Using the custom Dummy type
  it { is_expected.to contain_dummy('Test') }

  # This expectation fails!
  # Also note that the '!!!!! Autorequire for Dummy Type called for the test below' is never output so
  # Autorequires is NEVER called
  it { is_expected.to contain_notify('Test').that_requires(["Notify['notify1']"]) }

  # Even with an explicit cataloge compile, this expectation fails!
  it {
    is_expected.to compile

    is_expected.to contain_notify('Test').that_requires(["Notify['notify1']"])
  }
  it {
    is_expected.to compile.with_all_deps

    is_expected.to contain_notify('Test').that_requires(["Notify['notify1']"])
  }
end
  • The explicit requires never appears in the catalogue
  • The implicit autorequire on the Dummy type never appears in the catalogue

Also, using a simple puts statement it can be shown that the autorequire method is never called during the ... .that_requires(...) matcher. It only gets called during the explicit compile call. This is due to the cyclical checks which converts the catalogue into a graph. This is where the requires and autorequires are evaluated (https://github.com/puppetlabs/puppet/blob/0ff9cf7ff3a96b14c677ccaec73d48465a18c9c9/lib/puppet/graph/relationship_graph.rb#L22-L32)

Looking at this, there's no way I can test explicit or implicit requires using rspec-puppet.

@rodjek
Copy link
Owner

rodjek commented Dec 23, 2017

@glennsarti theres a couple of issues in play here. The first is the difference in how resource references are written in the manifest vs expressed in the catalogue. In the catalogue, resource titles are not quoted, so when you are testing the relationships, you need to specify them as Notify[notify1] rather than Notify['notify1']. I'm hoping to do away with this bit of confusion with the new syntax in rspec-puppet 3

The second is that you accidentally used contain_notify('Test') rather than contain_dummy('Test') 😉

diff --git a/spec/classes/testmodule_spec.rb b/spec/classes/testmodule_spec.rb
index aab1bcb..96bfa41 100644
--- a/spec/classes/testmodule_spec.rb
+++ b/spec/classes/testmodule_spec.rb
@@ -14,18 +14,18 @@ describe 'testmodule' do
   it { is_expected.to contain_notify('notify2') }
 
   # This expectation fails!
-  it { is_expected.to contain_notify('notify2').that_requires(["Notify['notify1']"]) }
+  it { is_expected.to contain_notify('notify2').that_requires(["Notify[notify1]"]) }
 
   # Even with an explicit cataloge compile, this expectation fails!
   it {
     is_expected.to compile
 
-    is_expected.to contain_notify('notify2').that_requires(["Notify['notify1']"])
+    is_expected.to contain_notify('notify2').that_requires(["Notify[notify1]"])
   }
   it {
     is_expected.to compile.with_all_deps
 
-    is_expected.to contain_notify('notify2').that_requires(["Notify['notify1']"])
+    is_expected.to contain_notify('notify2').that_requires(["Notify[notify1]"])
   }
 
   # ------------------------------------------- Using the custom Dummy type
@@ -34,18 +34,18 @@ describe 'testmodule' do
   # This expectation fails!
   # Also note that the '!!!!! Autorequire for Dummy Type called for the test below' is never output so
   # Autorequires is NEVER called
-  it { is_expected.to contain_notify('Test').that_requires(["Notify['notify1']"]) }
+  it { is_expected.to contain_dummy('Test').that_requires(["Notify[notify1]"]) }
 
   # Even with an explicit cataloge compile, this expectation fails!
   it {
     is_expected.to compile
 
-    is_expected.to contain_notify('Test').that_requires(["Notify['notify1']"])
+    is_expected.to contain_dummy('Test').that_requires(["Notify[notify1]"])
   }
   it {
     is_expected.to compile.with_all_deps
 
-    is_expected.to contain_notify('Test').that_requires(["Notify['notify1']"])
+    is_expected.to contain_dummy('Test').that_requires(["Notify[notify1]"])
   }
 end

@rodjek rodjek closed this as completed Dec 23, 2017
@glennsarti
Copy link
Author

Seems I was being a derp! Thanks Tim! I'll quickly review the docs and see if I missed anything, and if not, I can put up a PR to ammend the docs.

Thanks again.

@glennsarti
Copy link
Author

Derp level 11... All of what you said is in the big blue warning box. Apologies.

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

No branches or pull requests

2 participants