In tests verify config file contents instead of templatesymlink matcher tests.
In !3001 (merged), I changed tests checking for rendered file contents to be proper LWRP tests (for templatesymlink
resource) using matchers. This was to ensure we don't step into resources unnecessarily in tests and followed proper LWRP testing procedure. However, now I think that was a wrong move to make, especially because of the somewhat unique nature of templatesymlink
resource.
The main problem being it is very easy for someone to add variables to gitlab.rb and set default attributes but don't realize they have to explicitly use it in the config template - as seen in !3681 (comment 231173814), especially if they are not extremely familiar with omnibus-gitlab codebase. If our other tests suggested verifying the file contents and they referred those, I think this could've been caught early by the developers themselves.
I think we should make an exception to our "do not step into resources unnecessarily" guideline for templatesymlink
, and go back to checking the actual rendered file for its contents. This is because input to templatesymlink
and results of its actions can vary wildly compared to other resources.
@ibaum @twk3 I would like to hear your thoughts on this.
On that note, I also like what is being done in !3692 (merged) - instead of regexing the rendered file, let's load the rendered file as a Ruby implementation of yaml/toml, and then do equality checking. I am not sure of its performance implications yet, though.