correct test cases to properly check result

[felixdoerre]

When working with "file_line" and "ensure=>absent", I noticed that my existing manifests that were perfectly fine in 4.18.0, stopped working in 4.19.0. Digging deeper into the issue, I was able to identify commit 81d7d35 that caused the problem. In order to provide a minimal working example to describe the issue, I stumbled over the example from the "README.md":

file_line { 'bashrc_proxy':
  ensure             => present,
  path               => '/etc/bashrc',
  line               => 'export HTTP_PROXY=http://squid.puppetlabs.vm:3128',
  match              => '^export\ HTTP_PROXY\=',
  append_on_no_match => false,
}

I found out, that this example only works when the line to remove is exactly "export HTTP_PROXY=http://squid.puppetlabs.vm:3128" and does not do anything for a line like "export HTTP_PROXY=a".

I checked and saw that these test cases are exactly the ones run by "spec/unit/puppet/provider/file_line/ruby_spec.rb" which surprised me and I wondered how theses testcases could pass.

So after more debugging I saw that the problematic test cases don't even create the provider. When I corrected that, I noticed that the testcases were sill passing. I realized that the test cases don't check the "exists?" method, which is used when applying manifests with such resources. So I added that assertion as well.

Note that the corrected test cases will not pass, as the code from commit 81d7d35 causes such file_line resources to be handled incorrectly, which I did not attempt to correct.

[felixdoerre]

When poking around in this issue and trying to solve the problem at the initial implementation I discovered two things:
a) The code indicates that adding "replace => false" will have the same effect. Interestingly this is also indicated in a comment for the stackoverflow-question that caused this test cases to be added.
b) I tried to add the behavior for "match_for_absent" back in, that seems to be missing in the current implementation of the "ruby" provider. When comparing it with != 'true' the code seems to fulfill all test cases. However == 'false' does not seem to work. I am not sure how default values are handled here, and therefore not 100% sure that my proposed commit is correct.

[felixdoerre]

It seems that #825 has completely rewritten the "exists?" from file_line and on-the-fly been fixing this problem. So that leaves me to only correct the test cases. I will update the branch accordingly.

[felixdoerre]

And just in case someone did not already see the newer push from 5 days ago: I pushed an update to the branch so it only corrects the testcases. Can someone have a look at it?

[david22swan]

The provider is being created by the before statement.
It is run before each it and sets up the resources for the example.
You can see it at line 626 in the file.

In addition to this the 'exists?' is being called within the first iteration of the it within the overall context, with the expectation being that if it exists within the first one, it will exist within all the remaining ones due to the nature of the before statement.

Given this am unsure if your commit is necessary or adds anything to the code.

I apologize if I have been unclear in any manner and thank you for taking the time to make additions to the module.

[felixdoerre]

Thanks for the comment. Yes, I saw that a provider is created in line 626. However the test 655 wants to test a different resource (created in 656). Therefore I assumed that the provides has to be recreated. The same Pattern is already used in line 320 and then overridden in line 342, so I guessed that the same pattern needs to be used here. Am I wrong?

[felixdoerre]

I am not quite sure that I fully understand your point. I am sure that my changes are necessary, as the test cases previously failed to detect the regression introduced by 81d7d35 (that caused manifests on my server to not be applied correctly). Directly concerning your comment:
Yes, "exists?" is tested in line 644, however all following test cases replace the resource "under test" with a testcase specific one. (lines 656, 674, and more). You can see that I only touched testcases ("it"-blocks) where the resource defined by the context is specifically overridden. (e.g. for "it"-block 655 the 'generic' resource from 627 is overridden with a testcase-specific one in line 656)

[david22swan]

Ah, I see your point know.
That'll teach me to review PRs last thing before I leave at night.

[david22swan]

My apologies

[felixdoerre]

:-) thanks for doing review and thanks for merging