Skip to content

Conversation

tphoney
Copy link
Contributor

@tphoney tphoney commented Sep 27, 2018

No description provided.

@tphoney
Copy link
Contributor Author

tphoney commented Sep 27, 2018

Builds on #961

hlindberg and others added 20 commits September 27, 2018 11:46
Before this the expectation was that the value given as an argument
in the function call being tested would reach the function. That
assumption is false as the arguments to functions will be duplicates
when running for real.

This changes that expectation.
The tests in abs_spec.rb for 4x impl of function had several pending
tests for errors not being raised. With Puppet 6.0.0 the errors
are raised as expected.

This removes the "pending" status from those tests in abs_spec.rb
This fixes reverse_spec's test where it assumes a given argument
reaches the function as opposed to it being given a copy.
Before this, the pick_default_spec would fail on Puppet 6.0.0
because on Puppet 6.0.0 arguments given to a function are mapped
to comply with the 3x function calling spec.

The tests asserted that :undef and nil were returned as values (when
they were given as arguments) - however this never happens when the
function is called from the Puppet Language. (This the test was testing
assumptions that were not true).

The spec test is now modified so that when it runs for Puppet 6.0.0 and
later it will do the same mapping on the value as done on the input of
that value to the function before it compares the returned value against
the input. Thus retaining the spirit of the test.
Before this the test would fail on Puppet 6.0.0. This was because
testing logic compensated for hash not having defined order and
it tried to call function in a non API way.

Changing the test to simply use the testing framework fixes this
problem - and since Ruby 1.8 is no longer supported there is no need to
jump through a hoop to sort the result before comparing.
In Puppet 6.0.0 the getvar function is in Puppet and it has
new functionality as it supports an optional second argument
with a default value.

The new implementation also does not accept faulty variable
names - for example starting with '$'. Here the old implementation
simply returned undef because there is no way the language could find
a variable starting with '$' (the internal APIs are always without '$').

The tests are updated so that on Puppet 6.0.0 and later, it tests a
default value, and test for bad variable specification is changed to
simply test for a not found variable. (The real tests for the function
in puppet already tests that it raises an error for faulty variable).
Before this the test would fail because it used `subject_call` and
this ended up in an rspec_puppet compatibility method that
expects two parameters `scope` and `args`. The call was however made
without `scope` and this the arguments where taken as the given
scope - with undefined result.

The use of `call` is supposed to show a deprecation warning (maybe there
was one).

The fix in this commit is to change this to `subject.execute`
Before this the test would fail because the tests were calling
compatibility function `subject.call` and it requires a scope.

This is no fixed by using the `subject.execute` method instead
of `call.
Before this the test would fail because it used `subject.call` which
hits a compatibility method in rspec_puppet that expects to be called
with `scope`.

This is now fixed by instead using `subject.execute`
Before this the test would fail on Puppet 6.0.0 because
* it tried to ensure resources that were undefined
* using rspec_puppet deprecated (and broken) `subject.call` instead of
`subject.execute`
* Using `:undef` for undef value instead of `nil`
Before this the test would fail because it uses deprecated (and broken)
rspec_puppet method `subject.call`.

This is now fixed by using `subject.execute`.
Before this, the test would fail on Puppet 6.0.0 because it
assumes that :undef should be used as undef value when calling -
this is no longer true since the 3x function is wrapped in a
4x wrapper and it will map its arguments.

This is fixed by using a conditional value for undef depending
on Puppet version.
Before this the test would fail because it is using the deprecated (and
broken) rspec_puppet `subject.call` method instead of `subject.execute`.

Changing the method to `subject.execute` fixes the problem.
Before this, tests would fail for Puppet 6.0.0 because:
* use of deprecated (and broken) rspec_puppet `subject.call`
* assumption that `:undef` in nested structures were kept intact

In PUP-9180 (for Pupet 6.0.1) the behaviour of argument transformation
for 3.x functions is changed) - the fix here is therefore to simply
skip the tests for `:undef` as in Puppet 6.0.0 the `:undef` is
transformed to empty space (which is already skipped by tests).

The use of `subject.call` is also changed to `subject.execute`
This updates test using the deprecated (and broken) `subject.call` from
rspec_puppet to instead use `subject.execute`.
Before this the test would fail on Puppet 6.0.0:

* It assumed that user defined types did not have to exist when
declaring them (it is an error in Puppet 6.0.0).
* It assumed `:undef` would be taken as `nil`

This is fixed by defining the resource used for testing and by using
`nil` instead of `:undef` for puppet versions >= Puppet 6.0.0.
@tphoney tphoney force-pushed the hlindberg-FM-7388_fix-failing-tests-puppet-6 branch 2 times, most recently from 3d63534 to ab07c87 Compare September 27, 2018 12:41
@tphoney tphoney force-pushed the hlindberg-FM-7388_fix-failing-tests-puppet-6 branch from ab07c87 to 16b2629 Compare September 27, 2018 12:48
@pmcmaw pmcmaw changed the title Hlindberg fm 7388 fix failing tests puppet 6 (FM-7388) - Fixing unit tests for puppet 4, 5 and 6 Sep 27, 2018
@pmcmaw
Copy link
Contributor

pmcmaw commented Sep 27, 2018

LGTM. Thank you @tphoney and @hlindberg 🥇

@pmcmaw pmcmaw merged commit aae9f91 into puppetlabs:master Sep 27, 2018
@pmcmaw pmcmaw added the bugfix label Sep 27, 2018
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.

3 participants