(PUP-11655) Use run_mode for better platform independence #9294
Conversation
|
Thanks for taking this over @joshcooper. |
|
Test failures look relevant. |
joshcooper
force-pushed
the
PUP-11655
branch
2 times, most recently
from
842a949 to
0ea3dbc
Compare
| end | ||
| end | ||
| allow(provider.class).to receive(:which).with(provider_gem_cmd).and_return(provider_gem_cmd) | ||
| allow(Puppet.run_mode).to receive(:gem_cmd).and_return(provider_gem_cmd) |
There was a problem hiding this comment.
I think this mocking isn't working because of the order. Something loads the provider which resolves command. Then the stubbing happens, but no more lookup. I got it to work by stubbing the command lookup:
| allow(Puppet.run_mode).to receive(:gem_cmd).and_return(provider_gem_cmd) | |
| allow(provider.class).to receive(:command).with(:gemcmd).and_return(provider_gem_cmd) |
There was a problem hiding this comment.
As a rule of thumb, I avoid stubbing out the class I'm testing (including code that is inherited or mixed in). Otherwise, the tests can pass for the wrong reason. For example, if I stub out what Provider#command returns, then I could have a typo in the puppet_gem provider like:
command :gemcmd => "/whatever"and the test would still pass.
But the provider/command code is really complicated due to the load time vs runtime time behaviors you mention. So sometimes stubbing the command method is a necessary evil.
There was a problem hiding this comment.
Yes, stubbing and mocking in general are tools I love to avoid when possible. Here it really feels like you can't avoid it. Perhaps a separate test to assert it's not nil without mocking would help?
Don't stub methods on the provider we're testing like `validate_command`, `execute_gem_command` and `puppetservercmd`. As much as possible, only stub the call to `Puppet::Util::Execute.execution`. puppet_gem_spec was stubbing `Puppet::Util::Platform.windows?` to return false, which meant we were pretending to be POSIX when running specs on Windows.
| else | ||
| '/opt/puppetlabs/puppet/vendor_modules' | ||
| end | ||
| Puppet.run_mode.vendor_module_dir |
There was a problem hiding this comment.
To preserve API compatibility, I kept the Puppet.default_vendormoduledir. It also makes testing the default values for settings a bit easier, because of the way settings definitions are loaded once
joshcooper
force-pushed
the
PUP-11655
branch
2 times, most recently
from
ca80242 to
9133064
Compare
The run mode already determines the platform. Moving all platform specific paths into RunMode makes it easier to get a complete overview and change things where needed. Co-authored-by: Josh Cooper <[email protected]>
|
@ekohl I had to update your commit, could you review when you have some time? |
| end | ||
|
|
||
| def common_module_dir | ||
| "#{installdir}/puppet/modules" if installdir |
There was a problem hiding this comment.
I kept this as it used to be, but I was wondering if this is better
| "#{installdir}/puppet/modules" if installdir | |
| File.join(installdir, 'puppet', 'modules') if installdir |
| end | ||
|
|
||
| def vendor_module_dir | ||
| "#{installdir}\\puppet\\vendor_modules" if installdir |
There was a problem hiding this comment.
And the same here:
| "#{installdir}\\puppet\\vendor_modules" if installdir | |
| File.join(installdir, 'puppet', 'vendor_modules') if installdir |
The run mode already determines the platform. Moving all platform specific paths into RunMode makes it easier to get a complete overview and change things where needed.
@ekohl originally filed this #8938. I've retargeted his commit for
mainand updated some spec failures.