ADReplicationSiteLink: Add Integration tests, add setting 'options' #509

1800Zeta · 2019-09-23T06:31:05Z

Pull Request (PR) description

Add integration testing ability. Adds ability to enable options such as Change Notification Replication

This Pull Request (PR) fixes the following issues

Fixes ADReplicationSiteLink: Integration tests is missing for this resource #356
Fixes ADReplicationSiteLink: Support enable change notification replication #504

Task list

This change is

Unit tests also added

Tests for add, remove, rename default sites. Restores at the end back to default settings.

increase codecov score

Also had to tweak unit tests for updated logic

Integration Test completed before adding config

Options now being set to enable Change Notification Replication. Unit and Integration tests all completed.

codecov-io · 2019-09-23T06:40:51Z

Codecov Report

Merging #509 into dev will increase coverage by <1%.
The diff coverage is 100%.

@@         Coverage Diff          @@
##            dev   #509    +/-   ##
====================================
+ Coverage    98%    98%   +<1%     
====================================
  Files        23     23            
  Lines      3117   3161    +44     
  Branches     10     10            
====================================
+ Hits       3073   3117    +44     
  Misses       34     34            
  Partials     10     10

1800Zeta · 2019-10-07T05:46:37Z

Hi @johlju , are you able to review this and #510 at all please? I know there will be a conflict on the changelog.md when one is merged. Thank you

1800Zeta

Reviewable status: 4 of 7 files reviewed, 17 unresolved discussions (waiting on @1800Zeta and @johlju)

CHANGELOG.md, line 116 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

- Changes to ADReplicationSite
  - Added 'Description' attribute parameter ([issue #500](https://github.com/PowerShell/ActiveDirectoryDsc/issues/500)).
  - Added Integration testing ([issue #355](https://github.com/PowerShell/ActiveDirectoryDsc/issues/355)).
  - Correct value returned for RenameDefaultFirstSiteName ([issue #502](https://github.com/PowerShell/ActiveDirectoryDsc/issues/502)).

Should any of these be moved to the Unreleased section?

Done

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 74 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$Sitelink
$siteLink (lower-case 's', upper-case 'L')

Done.

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 76 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$SiteLinkOptions
$siteLinkOptions (lower-case 's'). Same for the ones below.

Done.

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 80 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

        else
        {
            $SiteLinkOptions = Get-EnabledOptions -OptionValue 0
        }
Should we need this if we can pass zero (0) to the parameter? 🤔

If nothing is configured it will return an empty/null value. If everything has been explicitly turned off you get a 0.

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 209 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$replacePraams
Can we use $replaceParameters to not use abbreviations? Throughout.

Done.

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 210 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$clearParams
Can we use $clearParameters to not use abbreviations? Throughout.

Removed as it was no longer needed

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 244 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$ChangeNotification
$changeNotification Lower-case 'c' . We should use camelCase for local variable namess and PascalCase for parameter variable names. Throughout.

Done.

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 273 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

'clear'
Should we have upper-case 'C' here?

Done.

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 273 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

'options'
Should we have upper-case 'O' here as the next row have?

Done.

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 464 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

OptionsValue
Should be OptionValue (no 's')

Should be without 's' even though the attribute it refers to is 'Options'? (have already switched them to camelCase.

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 472 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

# Parameter help description
We can remove this comment.

Done.

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 481 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$returnvalue
$returnValue (upper-case 'V'). Throughout.

Done.

1800Zeta

Reviewable status: 2 of 7 files reviewed, 17 unresolved discussions (waiting on @1800Zeta and @johlju)

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 489 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Could we instead do Bitwise AND? 🤔

Assuming $OptionValue contain the value 1-7.

if (1 -band $OptionValue)
{
    $returnvalue.USE_NOTIFY = $true
}

if (2 -band $OptionValue)
{
    $returnvalue.TWOWAY_SYNC = $true
}

if (4 -band $OptionValue)
{
    $returnvalue.DISABLE_COMPRESSION = $true
}

Done.

1800Zeta

Reviewable status: 2 of 7 files reviewed, 17 unresolved discussions (waiting on @1800Zeta and @johlju)

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 467 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Get-EnabledOptions
Can we please add a unit test that test this function separately. Add a separate Describe-block in the unit test for this resource, see example here https://github.com/PowerShell/SqlServerDsc/blob/da8ebe4539018c059bfb6ddbb4cf2bd9ca0ffc9d/Tests/Unit/MSFT_SqlSetup.Tests.ps1#L4077

Done.

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 527 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

ConvertTo-EnabledOptions
Can we please add a unit test that test this function separately. Add a separate Describe-block in the unit test for this resource, see example here https://github.com/PowerShell/SqlServerDsc/blob/da8ebe4539018c059bfb6ddbb4cf2bd9ca0ffc9d/Tests/Unit/MSFT_SqlSetup.Tests.ps1#L4077

Done.

1800Zeta

Reviewable status: 1 of 8 files reviewed, 17 unresolved discussions (waiting on @1800Zeta and @johlju)

DSCResources/MSFT_ADReplicationSiteLink/en-US/about_ADReplicationSiteLink.help.txt, line 96 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Configuration ADReplicationSiteLink_ModifyExistingReplicationSiteLink_Config
We should add this as an example to the correct example folder https://github.com/PowerShell/ActiveDirectoryDsc/tree/dev/Examples/Resources/ADReplicationSiteLink.

Then the New-DscResourcePowerShellHelp -$ModulePath . will generate this file correctly (when run manually)
https://github.com/PowerShell/DscResource.Tests/blob/dev/DscResource.DocumentationHelper/PowerShellHelp.psm1#L54
cd /source/ActiveDirectoryDsc
import-module ./DscResource.Tests/DscResource.DocumentationHelper/PowerShellHelp.psm1
New-DscResourcePowerShellHelp -$ModulePath .

Done.

Fixing a test

1800Zeta

Reviewable status: 1 of 8 files reviewed, 17 unresolved discussions (waiting on @johlju)

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 94 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

            OptionChangeNotification      = $SiteLinkOptions.USE_NOTIFY
            OptionTwoWaySync              = $SiteLinkOptions.TWOWAY_SYNC
            OptionDisableCompression      = $SiteLinkOptions.DISABLE_COMPRESSION

See previous comment.

Done.

johlju

I will verify the integration tests once these comments are resolved. Awesome work! 😃

Reviewed 5 of 5 files at r5.
Reviewable status: 6 of 8 files reviewed, 18 unresolved discussions (waiting on @1800Zeta)

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 74 at r4 (raw file):

Previously, 1800Zeta (Mark Lewis) wrote…

Done.

It looks this was resolved in all other rows but this one (line 74). :)

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 80 at r4 (raw file):

if ($Sitelink.Options -gt 0)

Then I suggest we change this check to be against and empty or null value instead of checking if the value is an integer.

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 464 at r4 (raw file):

Previously, 1800Zeta (Mark Lewis) wrote…

Should be without 's' even though the attribute it refers to is 'Options'? (have already switched them to camelCase.

This should be the same as the parameter name.

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 252 at r5 (raw file):

$TwoWaySync

We should use camelCase for local variable namess and PascalCase for parameter variable names. Throughout.

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 261 at r5 (raw file):

$DisableCompression

We should use camelCase for local variable namess and PascalCase for parameter variable names. Throughout.

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 475 at r5 (raw file):

$optionValue

$OptionValue. We should use PascalCase for parameter variable names.

DSCResources/MSFT_ADReplicationSiteLink/en-US/about_ADReplicationSiteLink.help.txt, line 96 at r5 (raw file):

ADReplicationSiteLink_EnableReplicationSiteLinkOptions_Config

This does not match the new example actual name. Did you run the cmdlet mentioned in previous review comment? 🤔

Examples/Resources/ADReplicationSiteLink/3-ADReplicationSiteLink_EnableOptions_Config.ps1, line 3 at r5 (raw file):

cd618e81-b903-4ae9-9dd0-ab794931505c

Please update the GUID with the output from New-Guid

Tests/Unit/MSFT_ADReplicationSiteLink.tests.ps1, line 262 at r5 (raw file):

It 'Should return $false with OptionChangeNotification $true is non compliant' {

Can we also test where it returns $true for each property?

Tests/Unit/MSFT_ADReplicationSiteLink.tests.ps1, line 334 at r5 (raw file):

Please remove the semi-colons in the hashtable.

Tests/Unit/MSFT_ADReplicationSiteLink.tests.ps1, line 338 at r5 (raw file):

} }

Please move the closing braces to each separate rows.

Tests/Unit/MSFT_ADReplicationSiteLink.tests.ps1, line 379 at r5 (raw file):

Please remove the semi-colons in the hashtable.

Tests/Unit/MSFT_ADReplicationSiteLink.tests.ps1, line 383 at r5 (raw file):

} }

Please move the closing braces to each separate rows.

Tests/Unit/MSFT_ADReplicationSiteLink.tests.ps1, line 406 at r5 (raw file):

Describe 'ADReplicationSiteLink\Get-EnabledOptions' {

Can we add a new line before this row?

Tests/Unit/MSFT_ADReplicationSiteLink.tests.ps1, line 408 at r5 (raw file):

"Should Return False False False"

Maybe we can say something like 'Should return the correct values in the hashtable'. Throughout.

Tests/Unit/MSFT_ADReplicationSiteLink.tests.ps1, line 408 at r5 (raw file):

"Should Return False False False"

We should use single-quote around strings where we can'.

Tests/Unit/MSFT_ADReplicationSiteLink.tests.ps1, line 490 at r5 (raw file):

It "Should return 0"{

Space before the open brace. Throughout below.

Tests/Unit/MSFT_ADReplicationSiteLink.tests.ps1, line 501 at r5 (raw file):

Context 'When Change Notification Replication is enabled' {

Can we add a blank line before this row?

1800Zeta

Reviewable status: 3 of 8 files reviewed, 18 unresolved discussions (waiting on @1800Zeta and @johlju)

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 464 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

This should be the same as the parameter name.

Done.

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 252 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$TwoWaySync
We should use camelCase for local variable namess and PascalCase for parameter variable names. Throughout.

Done.

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 261 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$DisableCompression
We should use camelCase for local variable namess and PascalCase for parameter variable names. Throughout.

Done.

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 475 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

$optionValue
$OptionValue. We should use PascalCase for parameter variable names.

Done.

DSCResources/MSFT_ADReplicationSiteLink/en-US/about_ADReplicationSiteLink.help.txt, line 96 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

ADReplicationSiteLink_EnableReplicationSiteLinkOptions_Config
This does not match the new example actual name. Did you run the cmdlet mentioned in previous review comment? 🤔

Done.

Examples/Resources/ADReplicationSiteLink/3-ADReplicationSiteLink_EnableOptions_Config.ps1, line 3 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

cd618e81-b903-4ae9-9dd0-ab794931505c
Please update the GUID with the output from New-Guid

Done.

Tests/Unit/MSFT_ADReplicationSiteLink.tests.ps1, line 334 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

;
Please remove the semi-colons in the hashtable.

Done.

Tests/Unit/MSFT_ADReplicationSiteLink.tests.ps1, line 338 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

} }
Please move the closing braces to each separate rows.

Done.

Tests/Unit/MSFT_ADReplicationSiteLink.tests.ps1, line 379 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

;
Please remove the semi-colons in the hashtable.

Done.

Tests/Unit/MSFT_ADReplicationSiteLink.tests.ps1, line 383 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

} }
Please move the closing braces to each separate rows.

Done.

Tests/Unit/MSFT_ADReplicationSiteLink.tests.ps1, line 406 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Describe 'ADReplicationSiteLink\Get-EnabledOptions' {
Can we add a new line before this row?

Done

Tests/Unit/MSFT_ADReplicationSiteLink.tests.ps1, line 408 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

"Should Return False False False"
Maybe we can say something like 'Should return the correct values in the hashtable'. Throughout.

Done

Tests/Unit/MSFT_ADReplicationSiteLink.tests.ps1, line 408 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

"Should Return False False False"
We should use single-quote around strings where we can'.

Done.

Tests/Unit/MSFT_ADReplicationSiteLink.tests.ps1, line 490 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

It "Should return 0"{
Space before the open brace. Throughout below.

Done.

Tests/Unit/MSFT_ADReplicationSiteLink.tests.ps1, line 501 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Context 'When Change Notification Replication is enabled' {
Can we add a blank line before this row?

Done.

1800Zeta

Reviewable status: 2 of 8 files reviewed, 18 unresolved discussions (waiting on @johlju)

DSCResources/MSFT_ADReplicationSiteLink/MSFT_ADReplicationSiteLink.psm1, line 80 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

if ($Sitelink.Options -gt 0)
Then I suggest we change this check to be against and empty or null value instead of checking if the value is an integer.

Done.

Tests/Unit/MSFT_ADReplicationSiteLink.tests.ps1, line 262 at r5 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

It 'Should return $false with OptionChangeNotification $true is non compliant' {
Can we also test where it returns $true for each property?

This is tested on line 200. Where in the desired state it tests all 3 properties.

johlju

I will run the integration test in by lab to verify them. But they look perfect. 😃 I will get back to you tomorrow.

Reviewed 4 of 4 files at r6.
Reviewable status: 6 of 8 files reviewed, all discussions resolved

johlju

Reviewed 2 of 7 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

johlju · 2019-10-15T13:12:34Z

@1800Zeta Thank you for this! The integration tests worked perfectly! 😃

1800Zeta · 2019-10-15T13:14:18Z

Thank you so much @johlju

1800Zeta and others added 17 commits September 12, 2019 13:50

Checking in to switch dev machines

419c4a4

Add description attribute to test-resource with unit tests

8dc90e7

Add Description to Set-TargetResource

e4b255d

Unit tests also added

Update test to increase codecov

dbb48d0

Added Integration Testing

de1cbba

Tests for add, remove, rename default sites. Restores at the end back to default settings.

Update Help Text and a small tidy up

f1fd66d

Update CHANGELOG.md

55b0c3d

Update MSFT_ADReplicationSite.Tests.ps1

ee8dbe9

increase codecov score

Updates based on review

07933f2

Set RenameDefaultFirstSiteName to false by default

823a1c5

Update CHANGELOG.md

ff0629a

Feedback request Pt2

bc5dd69

Also had to tweak unit tests for updated logic

Remove extra parentheses as requested

83b8971

Added Integration Tests

1ce3e66

Integration Test completed before adding config

Adding abilty to configure the Options on a sitelink

0e6e7ab

Complete with options being set and unit and integration tests

3808372

Options now being set to enable Change Notification Replication. Unit and Integration tests all completed.

Remove a debug pause from integration test

9905c6c

Merge remote-tracking branch 'upstream/dev' into ADReplicationSiteLink

8223ca3

johlju added waiting for author response The pull request is waiting for the author to respond to comments in the pull request. and removed waiting for author response The pull request is waiting for the author to respond to comments in the pull request. labels Sep 24, 2019

1800Zeta added 5 commits September 25, 2019 13:40

Adding some polish

a174ea5

Update MSFT_ADReplicationSiteLink.psm1

1986c03

Merge remote-tracking branch 'upstream/dev' into ADReplicationSiteLink

8102a0e

Update MSFT_ADReplicationSiteLink.tests.ps1

7ac926f

Update MSFT_ADReplicationSiteLink.tests.ps1

f90cebb

1800Zeta marked this pull request as ready for review September 27, 2019 10:39

johlju added the needs review The pull request needs a code review. label Sep 27, 2019

Update MSFT_ADReplicationSiteLink.psm1

c0a86e0

1800Zeta commented Oct 10, 2019

View reviewed changes

Update MSFT_ADReplicationSiteLink.psm1

46be221

1800Zeta commented Oct 10, 2019

View reviewed changes

Update MSFT_ADReplicationSiteLink.tests.ps1

5cbca3c

1800Zeta commented Oct 10, 2019

View reviewed changes

Update Examples

e5c8a31

1800Zeta commented Oct 10, 2019

View reviewed changes

Update 3-ADReplicationSiteLink_EnableOptions_Config.ps1

995596b

Fixing a test

1800Zeta commented Oct 10, 2019

View reviewed changes

johlju requested changes Oct 11, 2019

View reviewed changes

1800Zeta added 2 commits October 11, 2019 16:07

Further Feedback received

91367cd

Update MSFT_ADReplicationSiteLink.tests.ps1

26e776e

1800Zeta commented Oct 14, 2019

View reviewed changes

Update MSFT_ADReplicationSiteLink.psm1

76b9a2e

1800Zeta commented Oct 14, 2019

View reviewed changes

johlju reviewed Oct 14, 2019

View reviewed changes

johlju added needs review The pull request needs a code review. and removed waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. labels Oct 14, 2019

johlju approved these changes Oct 15, 2019

View reviewed changes

johlju added ready for merge The pull request was approved by the community and is ready to be merged by a maintainer. and removed needs review The pull request needs a code review. labels Oct 15, 2019

johlju changed the title ~~ADReplicationSiteLink Add Integration tests, add setting 'options'~~ ADReplicationSiteLink: Add Integration tests, add setting 'options' Oct 15, 2019

johlju merged commit c3c93ba into dsccommunity:dev Oct 15, 2019

johlju removed the ready for merge The pull request was approved by the community and is ready to be merged by a maintainer. label Oct 15, 2019

1800Zeta deleted the ADReplicationSiteLink branch October 15, 2019 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADReplicationSiteLink: Add Integration tests, add setting 'options' #509

ADReplicationSiteLink: Add Integration tests, add setting 'options' #509

1800Zeta commented Sep 23, 2019 •

edited

Loading

codecov-io commented Sep 23, 2019 •

edited

Loading

1800Zeta commented Oct 7, 2019

1800Zeta left a comment

1800Zeta left a comment

1800Zeta left a comment

1800Zeta left a comment

1800Zeta left a comment

johlju left a comment

1800Zeta left a comment

1800Zeta left a comment

johlju left a comment

johlju left a comment

johlju commented Oct 15, 2019

1800Zeta commented Oct 15, 2019

ADReplicationSiteLink: Add Integration tests, add setting 'options' #509

ADReplicationSiteLink: Add Integration tests, add setting 'options' #509

Conversation

1800Zeta commented Sep 23, 2019 • edited Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

codecov-io commented Sep 23, 2019 • edited Loading

Codecov Report

1800Zeta commented Oct 7, 2019

1800Zeta left a comment

Choose a reason for hiding this comment

1800Zeta left a comment

Choose a reason for hiding this comment

1800Zeta left a comment

Choose a reason for hiding this comment

1800Zeta left a comment

Choose a reason for hiding this comment

1800Zeta left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

1800Zeta left a comment

Choose a reason for hiding this comment

1800Zeta left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

johlju commented Oct 15, 2019

1800Zeta commented Oct 15, 2019

1800Zeta commented Sep 23, 2019 •

edited

Loading

codecov-io commented Sep 23, 2019 •

edited

Loading