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

(MODULES-2957) Fix registry_value tests #143

Merged
merged 1 commit into from
Dec 15, 2017

Conversation

glennsarti
Copy link
Contributor

@glennsarti glennsarti commented Dec 13, 2017

Previously the tests for the registry_value type were incomplete or were not
testing what they said they were. This commit;

  • Adds a compile test for when one resource is setting a default value and
    another is not, but shares the same name. A fix for this was implemented
    in 64bba67 however there were no tests for this behaviour
  • Fixed a bug in the canonicalising of path names which would append a backslash
    every time the method was called
  • Actually tested that multiple backslashes are removed on the path
  • Raised errors when an non default, but root only path was passed into a
    registry value. This is different to registry keys, where this is allowed.
  • Added tests for autorequiring of registry_key from registry_value resources
  • Added tests in the registry_value provider for creating and destroying default
    values
  • Added registry_value provider tests for case-insensitive exists? method and
    added for creating and destroying default values
  • Fixed typo in the registry_key type tests whereby it was actually testing the
    resource aliases, not the autorequire of the type

@glennsarti
Copy link
Contributor Author

Running through adhoc

@glennsarti
Copy link
Contributor Author

Adhoc passed. Ready for merge

@@ -0,0 +1,13 @@
class mixed_default_settings {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what mixed_default_settings means in this context?

Copy link
Contributor Author

@glennsarti glennsarti Dec 14, 2017

Choose a reason for hiding this comment

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

mixes default value_name and non-default value_name (default as in default registry key value

'hklm\Software\foo\\' vs 'hklm\Software\foo'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add comments into the test to explain this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add an ASCII art reg tree to show what's going on

expect(subject.autorequire).to eq([])
end

it 'should only autorequire the immediate ancestor registry_key resource' do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is slightly wrong. Autorequries grabs the first nearest parent key

it 'should only autorequire the immediate ancestor registry_key resource' do
catalog.add_resource(Puppet::Type.type(:registry_key).new(:path => 'hklm\Software', :catalog => catalog))
catalog.add_resource(Puppet::Type.type(:registry_key).new(:path => 'hklm\Software\foo', :catalog => catalog))
catalog.add_resource(Puppet::Type.type(:registry_key).new(:path => 'hklm\Software\foo\bar', :catalog => catalog))
Copy link
Contributor

@Iristyle Iristyle Dec 15, 2017

Choose a reason for hiding this comment

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

Add one new test here like:

catalog.add_resource(Puppet::Type.type(:registry_key).new(:path => 'hklm\Software\foo\bar\baz\ethan', :catalog => catalog))

And make sure that it autorequires nearest value of hklm\Software\foo\bar rather than non-existant hklm\Software\foo\bar\baz which doesn't exist.

@glennsarti glennsarti force-pushed the fix-tests branch 2 times, most recently from 79bbda1 to 563c54e Compare December 15, 2017 01:46
@glennsarti
Copy link
Contributor Author

@Iristyle Issues have been addressed. I also pulled in some tests I created in #142 into this PR. Updated the commit message accordingly.

@glennsarti
Copy link
Contributor Author

Ready for merge

Previously the tests for the registry_value type were incomplete or were not
testing what they said they were.  This commit;
* Adds a compile test for when one resource is setting a default value and
  another is not, but shares the same name.  A fix for this was implemented
  in 64bba67 however there were no tests for this behaviour
* Fixed a bug in the canonicalising of path names which would append a backslash
  every time the method was called
* Actually tested that multiple backslashes are removed on the path
* Raised errors when an non default, but root only path was passed into a
  registry value.  This is different to registry keys, where this is allowed.
* Added tests for autorequiring of registry_key from registry_value resources
* Added tests in the registry_value provider for creating and destroying default
  values
* Added registry_value provider tests for case-insensitive exists? method and
  added for creating and destroying default values
* Fixed typo in the registry_key type tests whereby it was actually testing the
  resource aliases, not the autorequire of the type
# |
# + foo
# + (default value) <-- This is the default value for a key called 'HKLM\Software\foo'.
# This is the 'hklm\Software\foo\\' resource
Copy link
Contributor

Choose a reason for hiding this comment

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

Big improvement 👍

@@ -60,12 +61,55 @@
it 'should validate the length of the value data'
it 'should be case-preserving'
it 'should be case-insensitive'
Copy link
Contributor

Choose a reason for hiding this comment

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

We can worry about this later.. but any reason to leave these since they don't do anything either?

@@ -51,7 +51,12 @@

describe "#exists?" do
it "should return true for a well known hive" do
reg_value = type.new(:path => 'hklm\SOFTWARE\Microsoft\Windows NT\CurrentVersion\SoftwareType', :provider => described_class.name)
reg_value = type.new(:title => 'hklm\SOFTWARE\Microsoft\Windows NT\CurrentVersion\SoftwareType', :provider => described_class.name)
Copy link
Contributor

@Iristyle Iristyle Dec 15, 2017

Choose a reason for hiding this comment

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

All the other tests in this file use :path rather than :title (given :path is the namevar they're currently equivalent) ... I assume this ties into the next PR? Just a little peculiar to see a different attribute for the type without any explanation for why :title is used here rather than :path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

copy-pasta typo. Should've been path

@Iristyle
Copy link
Contributor

A couple minor comments we can look at later - good enough for now given other changes pending.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants