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

Improve detection missing WhatIf support #2225

Closed
wants to merge 1 commit into from
Closed

Improve detection missing WhatIf support #2225

wants to merge 1 commit into from

Conversation

bheuvel
Copy link
Contributor

@bheuvel bheuvel commented Oct 14, 2014

From output of Whatif-run, do not replace line break by space, as this breaks the match if the break was done within a word.

From output of Whatif-run, do not replace line break by space, as this breaks the match if the break was done within a word.
@jaym
Copy link
Contributor

jaym commented Oct 14, 2014

@bheuvel Thanks!
Looks good, could you add a simple unit test for this? It goes in
.\spec\unit\util\dsc\local_configuration_manager_spec.rb

@bheuvel
Copy link
Contributor Author

bheuvel commented Oct 15, 2014

My first rspec unit test. Seems no problem, but two remarks/questions/opinions;

Do you want a separate test for the WhatIf; one (as currently) without the "unfortunate" linebreak, and one with the linebreak? So test both explicitly. Or just enter the "unfortunate" linebreak in the current test, which will effectively, yet implicitly, test both.

Second, due to pull #2190, the test_configuration will allways return a warning, and not an error. Having 5 minutes experience with this, it seems to me that this kinda invalidates some tests (at least that specific test line, used multiple times) using '.not_to raise_error', as it never will. So reliable testing different failure (all warning) scenario's doesn't seem reliable.
For example testing the "unfortunate" linebreak without the actual code adjustment, resulted in a good test; the code returned the "generic" PowerShell cmdlet error, which returns a warning, which is good enough for the 'missing WhatIf' test.

Resolution for that would be to use:

expect(Chef::Log).to receive(:warn).with('Received error while testing configuration due to resource not supporting \'WhatIf\'')

is that a good idea?

@jaym
Copy link
Contributor

jaym commented Oct 15, 2014

I don't generally like testing logging. I think a better way to do this would be to pull the regex out into its own function, similar to output_has_dsc_module_failure?. Then, you can test just that function

it 'should match whatif failure' do
  lcm.send(:output_has_whatif_failure?, no_whatif_lcm_output).should be_true
end

@adamedx
Copy link
Contributor

adamedx commented Oct 20, 2014

@bheuvel, let us know if you're able to make the refactoring change to avoid the expectation on logging. @jdmundrawala, you start on it please let @bheuvel know to avoid duplicate work.

@bheuvel
Copy link
Contributor Author

bheuvel commented Oct 23, 2014

@jdmundrawala , @adamedx , sorry, due to personal stuff (am) out of the running. Won't be able to assist/contribute :(

@jaym
Copy link
Contributor

jaym commented Oct 23, 2014

Thanks Bob.
I'm closing this PR as I merged it with #2264

@jaym jaym closed this Oct 23, 2014
@chef chef locked and limited conversation to collaborators Nov 16, 2017
@tas50 tas50 added Platform: Windows Triage: Needs Information Indicates an issue needs more information in order to work on it. and removed Area: Windows labels Jan 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Platform: Windows Triage: Needs Information Indicates an issue needs more information in order to work on it. Type: Enhancement Adds new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants