SPShellAdmins: Member comparison was case sensitive.

[ChristophHannappel]

Pull Request (PR) description

The Member comparison of SPShellAdmins was case sensitive. But Active Directory User and Groups are not.
This PR replaced the .Net .contains Method with the PowerShell Operator -contains and extended the unit tests.

This Pull Request (PR) fixes the following issues

Task list

• Added an entry to the change log under the Unreleased section of the file CHANGELOG.md.
  Entry should say what was changed and how that affects users (if applicable), and
  reference the issue being resolved (if applicable).
• Resource documentation added/updated in README.md.
• Resource parameter descriptions added/updated in README.md, schema.mof and comment-based
  help.
• Comment-based help added/updated.
• Localization strings added/updated in all localization files as appropriate.
• Examples appropriately added/updated.
• Unit tests added/updated. See DSC Community Testing Guidelines.
• Integration tests added/updated (where possible). See DSC Community Testing Guidelines.
• New/changed code adheres to DSC Community Style Guidelines.

---

This change is 

[ChristophHannappel]

@ykuijs the Build Pipeline has an issue with the GitVersion Task: Calculate ModuleVersion (GitVersion).
I think thats outside of my pr. Could you take a look?

[ykuijs]

@ChristophHannappel, will have a look!

Have had that issue before.....is an issue in the new version of gitversion. Solution was to specifically specify an older version

[ykuijs]

@ChristophHannappel Could you please implement the following change:
Add --version 5.12.0 to the end of this line:

SharePointDsc/azure-pipelines.yml

Line 29 in 13408f4

dotnet tool install --global GitVersion.Tool

The line should say dotnet tool install --global GitVersion.Tool --version 5.12.0

[ykuijs]

An HQRM check is failing. Will check the cause tomorrow.

[ChristophHannappel]

An HQRM check is failing. Will check the cause tomorrow.

Context SharePointDsc.Reverse.psm1 WARNING: The file SharePointDsc.Reverse.psm1 contains a suppression of the required PS Script Analyser rule PSAvoidUsingWriteHost. Please remove the rule suppression. [-] Should not suppress any required PS Script Analyzer rules 141ms Expected $false, but got $true. 125: $requiredRuleIsSuppressed | Should -BeFalse at <ScriptBlock>, D:\a\1\s\output\RequiredModules\DscResource.Test\0.16.3\Tests\QA\PSSAResource.common.v4.Tests.ps1: line 125

Thank you :)

[codecov]

Codecov Report

Attention: Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84%. Comparing base (b310e7d) to head (db9df9b).
Report is 13 commits behind head on master.

Files with missing lines	Patch %	Lines
...sources/MSFT_SPShellAdmins/MSFT_SPShellAdmins.psm1	83%	2 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##           master   #1440   +/-   ##
======================================
  Coverage      84%     84%           
======================================
  Files         145     145           
  Lines       22809   22809           
======================================
+ Hits        19236   19246   +10     
+ Misses       3573    3563   -10     

Files with missing lines	Coverage Δ
...sources/MSFT_SPShellAdmins/MSFT_SPShellAdmins.psm1	61% <83%> (+1%)	⬆️

---- 🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[johlju]

It was a change in module DscResource.Test to make that test run on just the source files. Prior to the change the HQRM test was run on both built module and source files but apparently due to a bug that test was never run on source files before. 🤔

If you want to keep Write-Host I suggest excluding that file for HQRM tests, add the relative path to the file here which I think should ignore the file:

SharePointDsc/build.yaml

Lines 93 to 94 in 13408f4

	ExcludeSourceFile:
	- output

Alternative I recommend changing Write-Host to:

Write-Information -MessageData 'MyMessage' -InformationAction 'Continue'

[ChristophHannappel]

If you want to keep Write-Host I suggest excluding that file for HQRM tests, add the relative path to the file here which I think should ignore the file:

SharePointDsc/build.yaml

Lines 93 to 94 in 13408f4

	ExcludeSourceFile:
	- output

I've added the file to the List, but that did not help.

@ykuijs would it be ok if I replaced the Write-Host with Write-Information as @johlju suggested?

[johlju]

Hmm strange I thought that would do it. @ChristophHannappel Can you test exclude the name SharePointDsc.Reverse to verify.

It should filter out the excluded files here:
https://github.com/dsccommunity/DscResource.Test/blob/14acda0bc0a6a9698bfb8fd41da42adfe08b09f0/source/Tests/QA/PSSAResource.common.v4.Tests.ps1#L82

Using the filter: https://github.com/dsccommunity/DscResource.Test/blob/14acda0bc0a6a9698bfb8fd41da42adfe08b09f0/source/Private/WhereSourceFileNotExcluded.ps1#L1

If above doesn't work then there must be a bug in DscResource.Test, need to debug to find it. I submit a bug in that repo. Maybe @dan-hughes have some input?

But do suggest replace Write-Host so hopefully @ykuijs is okay with that.

[johlju]

It was a change in module DscResource.Test to make that test run on just the source files. Prior to the change the HQRM test was run on both built module and source files but apparently due to a bug that test was never run on source files before. 🤔

Not true, that change hasn't been released yet. My bad. There must be something else going on in 0.16.3 release.

@ChristophHannappel can we test to see if the latest preview will help with this, there are a lot of improvements there. Change:

SharePointDsc/RequiredModules.psd1

Line 20 in 13408f4

'DscResource.Test' = 'latest'

too:

    'DscResource.Test'     = @{
        Version    = 'latest'
        Parameters = @{
            AllowPrerelease = $true
        }
    }

That will use the preview release 0.17.0-preview0003.

[ykuijs]

@ChristophHannappel, not sure what the impact will be if we remove Write-Host. I know we have been using Write-Host to specifically write to the screen instead of in the pipeline so we can use formatting, etc.

I see that the tests keep failing. In the last change you didn't specify the file extension. Can you please add that?

[johlju]

not sure what the impact will be if we remove Write-Host. I know we have been using Write-Host to specifically write to the screen instead of in the pipeline so we can use formatting, etc.

The command Write-Information uses the information stream to output messages to the console. But it cannot use text color without using ansi-sequences, but PS5 doesn't handle ansi-sequences either so text color is not possible. More input: https://stackoverflow.com/questions/38523369/write-host-vs-write-information-in-powershell-5

[johlju]

@ChristophHannappel thanks for testing the changes, it seems it didn't do anything. I won't have time to debubg DscResource.Test, but could try to do it this weekend - it could probably be a problem in the future for others as well.

@ykuijs I really suggest to move away from Write-Host for this particular problem, then the code will still be tested correctly and not excluded from all HQRM tests.

[dan-hughes]

HQRM issue is probably this commit.

I do not see any further changes to the Pester v4 HQRM tests. It is working as expected, required rules are not able to be disabled in the current test.

See here for a workaround but this disables required rule checking.

[johlju]

I added this issue dsccommunity/DscResource.Test#142 to track that it is seemingly not possible to exclude a source file from HQRM testing.

[dan-hughes]

dsccommunity/DscResource.Test#143 waiting to be merged that fixes the HQRM issue you are having.

The path in build.yml required is the path to the psm1 file relative to the source directory which in this case is the SharePointDsc directory inside the module.

[dan-hughes]

Therefore the entry you would require is:
- Modules/SharePointDsc.Reverse/SharePointDsc.Reverse.psm1

All the other entries including the output can be removed as these will not do anything.

[ykuijs]

@ChristophHannappel Once the other PR is merged: Can you update the build.yaml with the string suggested by @dan-hughes, so that the correct path will be excluded?

[ChristophHannappel]

@ChristophHannappel Once the other PR is merged: Can you update the build.yaml with the string suggested by @dan-hughes, so that the correct path will be excluded?

Sure thing.

@dan-hughes: will that also be a preview release?

[dan-hughes]

Yes. Unless one of the maintainers decides to trigger a full release.

[johlju]

We can test this PR with the preview and if it works I can release a full release of DscResource.Test. But I wont have time to review @dan-hughes PR until this weekend. To much to do at the day job 😊

[ChristophHannappel]

The

We can test this PR with the preview and if it works I can release a full release of DscResource.Test. But I wont have time to review @dan-hughes PR until this weekend. To much to do at the day job 😊

The Preview worked 🥳 Thank you very much.

@ykuijs I found another Bug in the SPSite Ressource which i will fix next. So if you can wait with the next Release both fixes could be shipped :)

[ykuijs]

@ChristophHannappel Great news!
Sure, no problem. Just submit the other fix in a PR and I will release a new version after merging that PR.

@johlju and @dan-hughes thanks a lot for your help in fixing this issue!

[dan-hughes]

I'd like to take credit, but the PR was pulled as it wasn't the correct fix.

Nice to know what the workaround is though!

[ChristophHannappel]

Seems like I have to fix some tests.
@ykuijs do you have a tip how to run the tests only for one Resource? So I can avoid the whole build.ps1 experience. Thank you

[dan-hughes]

Seems like I have to fix some tests. @ykuijs do you have a tip how to run the tests only for one Resource? So I can avoid the whole build.ps1 experience. Thank you

You can just specify the path to the test file in the Script key in build.yml.

It will not help with code coverage but it helps with test runtime.

[johlju]

t will not help with code coverage but it helps with test runtime.

If the code coverage is extensive it can be turned of by also passing -CodeCoverageThreshold 0

It should also possible to use Invoke-Pester directly after running ./build.ps1 -Task noop (onse for each session to set up the environment).

[ykuijs]

I believe it is also possible to run the .\build.ps1 -Tasks Build once and then just run the Pester ps1 file of the resource you would like to test.

[ykuijs]

@ChristophHannappel Now that all tests have completed successfully, can this PR be merged?

[ChristophHannappel]

@ykuijs Yes please.

At Everyone: Thank you for helping so quickly with my pipeline issues