Implement usage of Strict Mode in resources

[jensotto]

I think we should add Set-StrictMode with either version 1 or 2 to resources. This will make it easier to discover usage of uninitialized variables, references to non-existent properties of an object, function calls that use the syntax for calling methods, and variable without a name. I have seen issues in some of the resources that possibly would have been discovered by setting strict mode. Strict Mode will help increase the quality of the scripts.

Here is some more information: https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.core/set-strictmode https://blogs.technet.microsoft.com/heyscriptingguy/2014/12/03/enforce-better-script-practices-by-using-set-strictmode/

[andikrueger]

Is there a way that we can cover the rules that Set-StrictMode takes into account via the PSScriptAnalyser? That was my understanding so far and in VSCode it works quite well.

AFAIK Set-StrictMode does not take effect until the runtime of the scripts?

[ykuijs]

The impact of adding this can be quite big, since this might reveal issues in the code which was no problem before and after enabling this will start throwing errors. So this needs to get tested thoroughly.

Unused variables are indeed covered by the PSScriptAnalyzer, but I believe StrictMode does more.

@NikCharlebois What is your opinion in this?

[johlju]

One thing that strict mode does is that each variable must be declared before being used. That is probably the biggest thing (that I can think of) that will add to the technical debt and need to be resolved in all of the code. :thinking:

[ykuijs]

Just a thought: Isn't the Utils module a good place to add the StrictMode cmdlet. Since that is automatically loaded when importing the module, it is automatically loaded for all resources. Am I right?

[jensotto]

I don't think that will work as strict mode only works on the current scope and any child scopes.

[johlju]

But if the helper functions in "Util" are tested in unit tests then it should apply? If Strict mode is applied, will the unit tests start to fail (so we see it makes a difference)? 🤔

[NikCharlebois]

@ykuijs to me it is just common sense for us to use StrictMode v2.0. We are trying to encourage contributors to follow best practices, and want to make our code as clean as possible. Anything additional quality gates we can have in there the better if you ask me. Now, we would need to do a quick test run to see what the impact would be on all existing resources, but I am all in favor of using it.

I believe that all would be required for this to be checked upon running the tests in AppVeyor is to modify the YAML build definition to include the Set-StrictMode clause:

• ps: | Set-StrictMode -Version 2.0 $moduleName = 'SharePointDsc' $mainModuleFolder = "Modules$moduleName" Import-Module -Name "$env:APPVEYOR_BUILD_FOLDER\DscResource.Tests\AppVeyor.psm1" Invoke-AppveyorInstallTask

[johlju]

I think it would be better to enable strict mode in the test framework (the function that is run in each test, see test template in DscResource repo). Or, add it to a separate row in each test or/and code script file. Otherwise the strict mode would not be enabled when running the tests locally.

[stale[bot]]

This issue has been automatically marked as stale because it has not had activity from the community in the last 30 days. It will be closed if no further activity occurs within 10 days. If the issue is labelled with any of the work labels (e.g bug, enhancement, documentation, or tests) then the issue will not auto-close.

[jensotto]

I managed to get the StrictMode set by adding Set-StrictMode -Version 2.0 as the first line in initScript in UnitTestHelper.ps1 https://github.com/PowerShell/SharePointDsc/blob/b072ab3f00373e16c2b790d066f40fcd40d64076/Tests/Unit/UnitTestHelper.psm1#L56-L73 This way all tests will run the resources with strict mode set. It should probably also be added to other supporting script files too. In addition all unit tests should also have this set in each file. I don't see how that could be added elsewhere for local test runs. I get errors from all resources I've tested (I just tested a few) when I set strict mode to version 2, so there seems to be a lot of work to do to be able to actually implement this.

[stale[bot]]

This issue has been automatically marked as stale because it has not had activity from the community in the last 30 days. It will be closed if no further activity occurs within 10 days. If the issue is labelled with any of the work labels (e.g bug, enhancement, documentation, or tests) then the issue will not auto-close.

[jensotto]

I've implemented strict mode version 1 in #999 The use of strict mode uncovered a few bugs in some of the resources. At first I think we should just implement strict mode version 1 as using version 2 generated a lot more issues, and it will take time to fix all of them. Version 1 will still uncover many potential code issues.

[ykuijs]

#999 is merged, so strict mode is now enabled for all tests in version 1 mode. Next step is to move to version 2.

[stale[bot]]

This issue has been automatically marked as stale because it has not had activity from the community in the last 30 days. It will be closed if no further activity occurs within 10 days. If the issue is labelled with any of the work labels (e.g bug, enhancement, documentation, or tests) then the issue will not auto-close.