xArchive: Update to a high quality resource and add tests

[kwirkykat]

This is the high quality upgrade of xArchive for #160.

Unit tests, integration tests, end-to-end tests and examples have been added.
Documentation has been updated.

This fixes #127

---

This change is 

[mbreakey3]

Reviewed 2 of 25 files at r1.
Review status: 0 of 25 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

README.md, line 15 at r1 (raw file):

## Contributing

If you qould like to contribute to this module, please review the common DSC Resources [contributing guidelines](https://github.com/PowerShell/DscResource.Kit/blob/master/CONTRIBUTING.md).

'If you would'

---

DSCResources/MSFT_xArchive/MSFT_xArchive.psm1, line 21 at r1 (raw file):

if (Test-IsNanoServer)
{
    Add-Type -AssemblyName 'System.IO.Compression'

Why not just add this type above the if/else since they both need it?

---

DSCResources/MSFT_xArchive/MSFT_xArchive.psm1, line 38 at r1 (raw file):

            Destination: The specified destination.
            Ensure: Present if the archive at the specified path is expanded at the specified
                destination. Absent the archive at the specified path is not expanded at the

'Absent if the archive...'

---

DSCResources/MSFT_xArchive/MSFT_xArchive.psm1, line 55 at r1 (raw file):

        If a file does not match it will be considered not present.

        The default Checksum method is ModifiedDate.

I think this line can be removed from the Validate description

---

DSCResources/MSFT_xArchive/MSFT_xArchive.psm1, line 180 at r1 (raw file):

        If the file does not match and Ensure is specified as Present and Force is specified, the
        file at the desintation will be overwritten.
        If the file does not match and Ensure is specified as Absent, the file at the desintation

'desintation' lol

---

DSCResources/MSFT_xArchive/MSFT_xArchive.psm1, line 723 at r1 (raw file):

    Write-Verbose -Message ($script:localizedData.ClosingArchive -f $Path)
    $null = $Archive.Dispose()

is that technically what disposing the archive does? Close it? Not saying it's wrong, just curious

---

DSCResources/MSFT_xArchive/MSFT_xArchive.psm1, line 1203 at r1 (raw file):

    .PARAMETER Checksum
        The checksum method to use to determin if a file at the destination already matches a file

'to determine'

---

DSCResources/MSFT_xArchive/MSFT_xArchive.psm1, line 1245 at r1 (raw file):

        foreach ($archiveEntry in $archiveEntries)
        {
            $archivEntryFullName = Get-ArchiveEntryFullName -ArchiveEntry $archiveEntry

$archiveEntryFullName

---

DSCResources/MSFT_xArchive/MSFT_xArchive.psm1, line 1275 at r1 (raw file):

                if ($archivEntryFullName.EndsWith('\'))
                {

Could you add a comment here about what this check is checking? It's a little hard to follow

---

DSCResources/MSFT_xArchive/MSFT_xArchive.psm1, line 1415 at r1 (raw file):

        foreach ($archiveEntry in $archiveEntries)
        {
            $archivEntryFullName = Get-ArchiveEntryFullName -ArchiveEntry $archiveEntry

$archiveEntryFullName

---

Comments from Reviewable

[kwirkykat]

Review status: 0 of 25 files reviewed at latest revision, 10 unresolved discussions.

---

README.md, line 15 at r1 (raw file):

Previously, mbreakey3 (Mariah) wrote…

'If you would'

Done.

---

DSCResources/MSFT_xArchive/MSFT_xArchive.psm1, line 21 at r1 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Why not just add this type above the if/else since they both need it?

Done.

---

DSCResources/MSFT_xArchive/MSFT_xArchive.psm1, line 38 at r1 (raw file):

Previously, mbreakey3 (Mariah) wrote…

'Absent if the archive...'

Done.

---

DSCResources/MSFT_xArchive/MSFT_xArchive.psm1, line 55 at r1 (raw file):

Previously, mbreakey3 (Mariah) wrote…

I think this line can be removed from the Validate description

Done.

---

DSCResources/MSFT_xArchive/MSFT_xArchive.psm1, line 180 at r1 (raw file):

Previously, mbreakey3 (Mariah) wrote…

'desintation' lol

Done.

---

DSCResources/MSFT_xArchive/MSFT_xArchive.psm1, line 723 at r1 (raw file):

Previously, mbreakey3 (Mariah) wrote…

is that technically what disposing the archive does? Close it? Not saying it's wrong, just curious

Part of the archive disposal is "all underlying streams are closed and no longer available for subsequent write operations." Technically it also removes any other resources held by the archive, not just the streams though.

So it wouldn't be wrong to call this function Remove-Archive or Invoke-ArchiveDispose.
But in this case I prefer Close-Archive because we also have an Open-Archive and I want it clear here that if it is 'opened' it needs to be 'closed', and it is actually closing the stream that we opened with Open-Archive.

---

DSCResources/MSFT_xArchive/MSFT_xArchive.psm1, line 1203 at r1 (raw file):

Previously, mbreakey3 (Mariah) wrote…

'to determine'

Done.

---

DSCResources/MSFT_xArchive/MSFT_xArchive.psm1, line 1245 at r1 (raw file):

Previously, mbreakey3 (Mariah) wrote…

$archiveEntryFullName

Done.

---

DSCResources/MSFT_xArchive/MSFT_xArchive.psm1, line 1275 at r1 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Could you add a comment here about what this check is checking? It's a little hard to follow

I put it in a more descriptive variable instead since that's what I did down in the remove function

---

DSCResources/MSFT_xArchive/MSFT_xArchive.psm1, line 1415 at r1 (raw file):

Previously, mbreakey3 (Mariah) wrote…

$archiveEntryFullName

Done.

---

Comments from Reviewable

[mbreakey3]

Reviewed 21 of 25 files at r1, 3 of 4 files at r2.
Review status: 24 of 25 files reviewed at latest revision, 33 unresolved discussions.

---

DSCResources/MSFT_xArchive/MSFT_xArchive.psm1, line 209 at r2 (raw file):

        the resource will throw an error that the file at the desintation cannot be overwritten.
        If the file does not match and Ensure is specified as Present and Force is specified, the
        file at the desintation will be overwritten.

one more spot - 'desintation'

---

DSCResources/MSFT_xArchive/MSFT_xArchive.schema.mof, line 7 at r2 (raw file):

  [Key, Description("The path where the specified archive file should be expanded to or removed from.")] String Destination;
  [Write, Description("Specifies whether or not the expanded content of the archive file at the specified path should exist at the specified destination. To update the specified destination to have the expanded content of the archive file at the specified path, specify this property as Present. To remove the expanded content of the archive file at the specified path from the specified destination, specify this property as Absent. The default value is Present."), ValueMap{"Present", "Absent"}, Values{"Present", "Absent"}] String Ensure;
  [Write, Description("Specifies whether or not to validate that a file at the destination with the same name as a file in the archive actually matches that corresponding file in the archive by the specified checksum method. If the file does not match and Ensure is specified as Present and Force is not specified, the resource will throw an error that the file at the desintation cannot be overwritten. If the file does not match and Ensure is specified as Present and Force is specified, the file at the desintation will be overwritten. If the file does not match and Ensure is specified as Absent, the file at the desintation will not be removed. The default Checksum method is ModifiedDate. The default value is false.")] Boolean Validate;

Can you remove 'The default Checksum method is ModifiedDate.' here too

---

DSCResources/MSFT_xArchive/en-US/MSFT_xArchive.schema.mfl, line 7 at r2 (raw file):

  [Key,Description("The path where the specified archive file should be expanded to or removed from.") : Amended] String Destination;
  [Description("Specifies whether or not the expanded content of the archive file at the specified path should exist at the specified destination. To update the specified destination to have the expanded content of the archive file at the specified path, specify this property as Present. To remove the expanded content of the archive file at the specified path from the specified destination, specify this property as Absent. The default value is Present.") : Amended] String Ensure;
  [Description("Specifies whether or not to validate that a file at the destination with the same name as a file in the archive actually matches that corresponding file in the archive by the specified checksum method. If the file does not match and Ensure is specified as Present and Force is not specified, the resource will throw an error that the file at the desintation cannot be overwritten. If the file does not match and Ensure is specified as Present and Force is specified, the file at the desintation will be overwritten. If the file does not match and Ensure is specified as Absent, the file at the desintation will not be removed. The default Checksum method is ModifiedDate. The default value is false.") : Amended] Boolean Validate;

Remove Checksum comment here as well

---

DSCResources/MSFT_xArchive/en-US/MSFT_xArchive.strings.psd1, line 13 at r2 (raw file):

    RemovingPSDrive = Removing the mounted PSDrive {0}...

    DestinationExists = A directory already exists at the destination path {0} .

Extra space before the period at the end.

---

DSCResources/MSFT_xArchive/en-US/MSFT_xArchive.strings.psd1, line 33 at r2 (raw file):

    TestingIfFileMatchesArchiveEntryByChecksum = Testing if the file at {0} matches the archive entry at {1} by the checksum method {2}...
    FileTimeStampsDoNotMatch = The timestamps of the corresponding archive entry and the file at the path {0} do not match according to the checksum method {1}.
    ComparingHashes = Comparing the hash of the file at {0} to the archive entry {1} with the hash algorithm {2}...

'Comparing the hashes of the file at {0} and the archive entry at {1} with the hash algorithm {2}...'
or
'Comparing the hash of the file at {0} to the hash of the archive entry at {1} with the hash algorithm {2}...'

---

DSCResources/MSFT_xArchive/en-US/MSFT_xArchive.strings.psd1, line 35 at r2 (raw file):

    ComparingHashes = Comparing the hash of the file at {0} to the archive entry {1} with the hash algorithm {2}...
    FileHashesDoNotMatch = The hash values of the corresponding archive entry and the file at the path {0} do not match according to the checksum method {1}.
    FilesMatchesArchiveEntryByChecksum = The file at {0} matches the archive entry at {1} by the checksum method {2}.

'FileMatchesArchiveEntryByChecksum' (instead of 'Files....')

---

DSCResources/MSFT_xArchive/en-US/MSFT_xArchive.strings.psd1, line 36 at r2 (raw file):

    FileHashesDoNotMatch = The hash values of the corresponding archive entry and the file at the path {0} do not match according to the checksum method {1}.
    FilesMatchesArchiveEntryByChecksum = The file at {0} matches the archive entry at {1} by the checksum method {2}.
    FilesDoesNotMatchArchiveEntryByChecksum = The file at {0} does not match the archive entry at {1} by the checksum method {2}.

'FileDoesNotMatchArchiveEntryByChecksum'

---

Examples/Sample_xArchive_ExpandArchiveChecksumAndForce.ps1, line 8 at r2 (raw file):

        Since Validate is specified as $true and the Checksum parameter is specified as SHA-256, the
        resource will check if the SHA-256 hash of the file in the archive matches the SHA-256 hash
        of the correspnding file at the destination and replace any files that do not match.

'corresponding'

---

Examples/Sample_xArchive_ExpandArchiveChecksumAndForce.ps1, line 22 at r2 (raw file):

        xArchive Archive3
        {
            Path = 'C:\ArchivePath\Archive.zip'

Maybe change to 'C:\TestArchivePath\Archive.zip' and 'C:\TestDestinationPath\Destination', just as an extra precaution since this is set to override files

---

Examples/Sample_xArchive_ExpandArchiveDefaultValidationAndForce.ps1, line 22 at r2 (raw file):

        xArchive Archive2
        {
            Path = 'C:\ArchivePath\Archive.zip'

Same comment as above with naming

---

Examples/Sample_xArchive_ExpandArchiveNoValidationCredential.ps1, line 27 at r2 (raw file):

    Node localhost
    {
        xArchive Archive1

Archive6

---

Examples/Sample_xArchive_RemoveArchiveChecksum.ps1, line 8 at r2 (raw file):

        Since Validate is specified as $true and the Checksum parameter is specified as SHA-256, the
        resource will check if the SHA-256 hash of the file in the archive matches the SHA-256 hash
        of the correspnding file at the destination and will not remove any files that do not match.

'corresponding'

---

Examples/Sample_xArchive_RemoveArchiveChecksum.ps1, line 18 at r2 (raw file):

        xArchive Archive4
        {
            Path = 'C:\ArchivePath\Archive.zip'

change path names

---

Examples/Sample_xArchive_RemoveArchiveNoValidation.ps1, line 18 at r2 (raw file):

        xArchive Archive5
        {
            Path = 'C:\ArchivePath\Archive.zip'

change path names

---

Tests/Integration/MSFT_xArchive.EndToEnd.Tests.ps1, line 127 at r2 (raw file):

    AfterAll {
        Exit-DscResourceTestEnvironment -TestEnvironment $script:testEnvironment

I kind of like this better in the old structure putting the Enter-DscResourceTestEnvironment at the top and the Exit in the finally block - this gets lost within the rest of the code. Is there a reason you switched to this structure?

---

Tests/Integration/MSFT_xArchive.EndToEnd.Tests.ps1, line 135 at r2 (raw file):

        $destination = Join-Path -Path $TestDrive -ChildPath 'NonExistentDestinationForExpand'

        It 'Destination should not exist before configuration' {

It 'Should return $false that the destination exists'

---

Tests/Integration/MSFT_xArchive.EndToEnd.Tests.ps1, line 157 at r2 (raw file):

        }

        It 'Destination should exist after configuration' {

It 'Should return true that the destination exists after configuration'

---

Tests/Integration/MSFT_xArchive.EndToEnd.Tests.ps1, line 161 at r2 (raw file):

        }

        It 'File structure of destination should match the file structure of the archive' {

It 'Should return that the file structure of destination matches the file structure of the archive'

---

Tests/Integration/MSFT_xArchive.EndToEnd.Tests.ps1, line 205 at r2 (raw file):

            if ($otherItemIsDirectory)
            {
                It "Other item under destination $otherItemName should exist as a directory before configuration" {

It "Should return True that the item under destination $otherItemName should exist as a directory before configuration"

---

Tests/Integration/MSFT_xArchive.EndToEnd.Tests.ps1, line 211 at r2 (raw file):

            else
            {
                It "Other item under destination $otherItemName should exist as a file before configuration" {

same as above

---

Tests/Integration/MSFT_xArchive.EndToEnd.Tests.ps1, line 215 at r2 (raw file):

                }

                It "Other file under destination $otherItemName should have the expected content before configuration" {

It "Should return that the file under destination $otherItemName has the expected contents before configuration"

---

Tests/Integration/MSFT_xArchive.EndToEnd.Tests.ps1, line 362 at r2 (raw file):

                & $configurationName -OutputPath $TestDrive @archiveParameters
                Start-DscConfiguration -Path $TestDrive -ErrorAction 'Stop' -Wait -Force
            } | Should Throw

Do you know the actual exception it will throw here? - That would be good to include

---

Tests/Integration/MSFT_xArchive.EndToEnd.Tests.ps1, line 598 at r2 (raw file):

        }

        # Test-FileStructuresMatch -SourcePath $script:testArchiveFilePathWithoutExtension -DestinationPath $emptyDestination

Is this line not being used anymore? (here and in all of the tests)

---

Tests/Integration/MSFT_xArchive.EndToEnd.Tests.ps1, line 830 at r2 (raw file):

        It 'File structure of destination should not match the file structure of the archive' {
            Test-FileStructuresMatch -SourcePath $script:testArchiveFilePathWithoutExtension -DestinationPath $destination | Should Be $false

Wait, shouldn't all the extra files have been deleted?

---

Tests/Integration/MSFT_xArchive.Integration.Tests.ps1, line 40 at r2 (raw file):

        $destinationDirectoryPath = Join-Path -Path $TestDrive -ChildPath $destinationDirectoryName

        It 'File structure and contents of the destination should not match the file structure and contents of the archive before Set-TargetResource' {

Can you change these to 'Should statements'

---

Tests/Integration/MSFT_xArchive.Integration.Tests.ps1, line 86 at r2 (raw file):

        }

        It 'Test-TargetResource with Ensure as Present should return false before Set-TargetResource' {

This doesn't look like it's actually testing removing an archive since Test-TargetResouce is returning False when Ensure is set to Present and True when Ensure is set to Absent.

---

Tests/Integration/MSFT_xArchive.Integration.Tests.ps1, line 222 at r2 (raw file):

        }

        It 'Test-TargetResource with Ensure as Present should return false before Set-TargetResource' {

Again, this doesn't look like it's actually testing the removal of an Archive.

---

Tests/Integration/MSFT_xArchive.Integration.Tests.ps1, line 528 at r2 (raw file):

            }

            It 'File structure and contents of the destination should not match the file structure and contents of the archive before Set-TargetResource' {

'...of the archive after Set-TargetResource'

---

Tests/Integration/MSFT_xArchive.Integration.Tests.ps1, line 532 at r2 (raw file):

            }

            { Set-TargetResource -Ensure 'Present' -Path $zipFilePath -Destination $destinationDirectoryPath -Validate $true -Checksum $possibleChecksumValue } | Should Throw

Are all the extra tests above this one really necessary if this is supposed to just throw anyway? - I know these are the old tests, but it does make it a little hard to follow. - Also can we add what should actually be thrown here.

---

Tests/Integration/MSFT_xArchive.Integration.Tests.ps1, line 536 at r2 (raw file):

        Context "Remove an archive with an edited file, Validate specified, and Checksum specified as $possibleChecksumValue" {
            $zipFileName = 'ChecksumWithModifiedFile'

Could these be declared above the foreach loop since they're used in every Context block?

---

Tests/Integration/MSFT_xArchive.Integration.Tests.ps1, line 580 at r2 (raw file):

            }

            It 'Test-TargetResource with Ensure as Present should return false before Set-TargetResource' {

Again - this is looking like the archive is already absent.

---

Tests/Integration/MSFT_xArchive.Integration.Tests.ps1, line 620 at r2 (raw file):

            }

            It 'File structure and contents of the destination should not match the file structure and contents of the archive before Set-TargetResource' {

'... of the archive after Set-TargetResource'

---

Tests/Integration/MSFT_xArchive.TestHelper.psm1, line 68 at r2 (raw file):

    .SYNOPSIS
        Creates a new zip file with the specified name and file structure under the specified parent path.
        Returns the file path the to compressed zip file.

'Returns the file path to the compressed zip file.'

---

Comments from Reviewable

[kwirkykat]

Still working on the integration tests, but the rest is done

---

Review status: 12 of 25 files reviewed at latest revision, 33 unresolved discussions.

---

DSCResources/MSFT_xArchive/MSFT_xArchive.psm1, line 209 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

one more spot - 'desintation'

Done.

---

DSCResources/MSFT_xArchive/MSFT_xArchive.schema.mof, line 7 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Can you remove 'The default Checksum method is ModifiedDate.' here too

Done.

---

DSCResources/MSFT_xArchive/en-US/MSFT_xArchive.schema.mfl, line 7 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Remove Checksum comment here as well

Done.

---

DSCResources/MSFT_xArchive/en-US/MSFT_xArchive.strings.psd1, line 13 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Extra space before the period at the end.

Done.

---

DSCResources/MSFT_xArchive/en-US/MSFT_xArchive.strings.psd1, line 33 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

'Comparing the hashes of the file at {0} and the archive entry at {1} with the hash algorithm {2}...'
or
'Comparing the hash of the file at {0} to the hash of the archive entry at {1} with the hash algorithm {2}...'

Done.

---

DSCResources/MSFT_xArchive/en-US/MSFT_xArchive.strings.psd1, line 35 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

'FileMatchesArchiveEntryByChecksum' (instead of 'Files....')

Wasn't used. Removed this string.

---

DSCResources/MSFT_xArchive/en-US/MSFT_xArchive.strings.psd1, line 36 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

'FileDoesNotMatchArchiveEntryByChecksum'

Done.

---

Examples/Sample_xArchive_ExpandArchiveChecksumAndForce.ps1, line 8 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

'corresponding'

Done.

---

Examples/Sample_xArchive_ExpandArchiveChecksumAndForce.ps1, line 22 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Maybe change to 'C:\TestArchivePath\Archive.zip' and 'C:\TestDestinationPath\Destination', just as an extra precaution since this is set to override files

Done.

---

Examples/Sample_xArchive_ExpandArchiveDefaultValidationAndForce.ps1, line 22 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Same comment as above with naming

Done.

---

Examples/Sample_xArchive_ExpandArchiveNoValidationCredential.ps1, line 27 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Archive6

This example is still 1, but the others have changed so there is now 1-6

---

Examples/Sample_xArchive_RemoveArchiveChecksum.ps1, line 8 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

'corresponding'

Done.

---

Examples/Sample_xArchive_RemoveArchiveChecksum.ps1, line 18 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

change path names

Done.

---

Examples/Sample_xArchive_RemoveArchiveNoValidation.ps1, line 18 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

change path names

Done.

---

Tests/Integration/MSFT_xArchive.EndToEnd.Tests.ps1, line 127 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

I kind of like this better in the old structure putting the Enter-DscResourceTestEnvironment at the top and the Exit in the finally block - this gets lost within the rest of the code. Is there a reason you switched to this structure?

I like it better in this structure. It fits into the Pester structure better rather than looking like a hack that we tacked on, and it looks cleaner to me with less brackets and less nesting. We also have all the imports together in one place this way.

---

Tests/Integration/MSFT_xArchive.EndToEnd.Tests.ps1, line 135 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

It 'Should return $false that the destination exists'

I have tried changing all these test names you are commenting on to statements beginning with 'Should', but I think they all read much better the way they are now. I think we need to relax our insistence that these all begin with 'Should' and actually think about what is being tested here.

Especially this one in particular since there is nothing that the code should or should not have done at this point. We are just ensuring that the test set-up is correct.

Come talk to me if you still think these test names are really worth changing.

---

Tests/Integration/MSFT_xArchive.EndToEnd.Tests.ps1, line 157 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

It 'Should return true that the destination exists after configuration'

See comment above.

---

Tests/Integration/MSFT_xArchive.EndToEnd.Tests.ps1, line 161 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

It 'Should return that the file structure of destination matches the file structure of the archive'

I did add 'after configuration' to this one to clarify it, but see comment above.

---

Tests/Integration/MSFT_xArchive.EndToEnd.Tests.ps1, line 205 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

It "Should return True that the item under destination $otherItemName should exist as a directory before configuration"

See above comment

---

Tests/Integration/MSFT_xArchive.EndToEnd.Tests.ps1, line 211 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

same as above

See above comment

---

Tests/Integration/MSFT_xArchive.EndToEnd.Tests.ps1, line 215 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

It "Should return that the file under destination $otherItemName has the expected contents before configuration"

See above comment

---

Tests/Integration/MSFT_xArchive.EndToEnd.Tests.ps1, line 362 at r2 (raw file):

We don't know which file will throw the error, so we will only check that an error was thrown rather than checking the specific error message

No that's why this comment is there

---

Tests/Integration/MSFT_xArchive.EndToEnd.Tests.ps1, line 598 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Is this line not being used anymore? (here and in all of the tests)

Replaced with the actual file structure test

---

Tests/Integration/MSFT_xArchive.EndToEnd.Tests.ps1, line 830 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Wait, shouldn't all the extra files have been deleted?

Nope. The Archive resource won't remove any extra files that were added to the destination.
It also will not remove the destination directory itself.

---

Tests/Integration/MSFT_xArchive.TestHelper.psm1, line 68 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

'Returns the file path to the compressed zip file.'

Done.

---

Comments from Reviewable

[kwirkykat]

Review status: 11 of 25 files reviewed at latest revision, 33 unresolved discussions.

---

Tests/Integration/MSFT_xArchive.Integration.Tests.ps1, line 40 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Can you change these to 'Should statements'

Talked about this offline

---

Tests/Integration/MSFT_xArchive.Integration.Tests.ps1, line 86 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

This doesn't look like it's actually testing removing an archive since Test-TargetResouce is returning False when Ensure is set to Present and True when Ensure is set to Absent.

Fixed! Good catch!

---

Tests/Integration/MSFT_xArchive.Integration.Tests.ps1, line 222 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Again, this doesn't look like it's actually testing the removal of an Archive.

Done.

---

Tests/Integration/MSFT_xArchive.Integration.Tests.ps1, line 528 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

'...of the archive after Set-TargetResource'

This is still before Set-TargetResource?

---

Tests/Integration/MSFT_xArchive.Integration.Tests.ps1, line 532 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Are all the extra tests above this one really necessary if this is supposed to just throw anyway? - I know these are the old tests, but it does make it a little hard to follow. - Also can we add what should actually be thrown here.

Yes because I need to make sure that the machine is in the correct state before Set-TargetResource is called

---

Tests/Integration/MSFT_xArchive.Integration.Tests.ps1, line 536 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Could these be declared above the foreach loop since they're used in every Context block?

Moved.

---

Tests/Integration/MSFT_xArchive.Integration.Tests.ps1, line 580 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Again - this is looking like the archive is already absent.

This one is actually correct.
Because we edited, the resource will think that the archive is not completely present at the destination.

But I added tests before the edit as well to clarify what should be happening here.
That brought up a different bug with the dates (they weren't all getting normalized to the same format)
Unfortunately that also brought up that the tests weren't quite set up correctly.
For 2 of the checksum methods, the timestamp of the file at the destination must match the timestamp of the file compressed inside the archive. In order to retrieve that timestamp to set those of the files at the destination correctly, I have to open the archive. Which sucks. But I tried every other timestamp I could (the zip file before compression, the entire zip file, the zip file after expansion) and none work consistently except this one.

---

Tests/Integration/MSFT_xArchive.Integration.Tests.ps1, line 620 at r2 (raw file):

Previously, mbreakey3 (Mariah) wrote…

'... of the archive after Set-TargetResource'

Done.

---

Comments from Reviewable

[mbreakey3]

Reviewed 1 of 14 files at r3.
Review status: 12 of 25 files reviewed at latest revision, 41 unresolved discussions.

---

Tests/Unit/MSFT_xArchive.Tests.ps1, line 933 at r3 (raw file):

            }

            Context '*Valid archive path and destination specified, destination does not exist, Ensure specified as Absent, Validate specified as true, and Checksum specified' {

Is the '*' here supposed to be?

---

Tests/Unit/MSFT_xArchive.Tests.ps1, line 1837 at r3 (raw file):

                }
            }
        }

I would add a test for the Checksum parameter name being shorter than 3 chars as well.

---

Tests/Unit/MSFT_xArchive.Tests.ps1, line 3438 at r3 (raw file):

                }

                It 'Should not attempt to set the last access time of the file at the specified destination' {

'Should set the last access time...'

---

Tests/Unit/MSFT_xArchive.Tests.ps1, line 3450 at r3 (raw file):

                }

                It 'Should not attempt to set the creation time of the file at the specified destination' {

'Should set the creation time...'

---

Tests/Unit/MSFT_xArchive.Tests.ps1, line 4248 at r3 (raw file):

                }

                It 'Should find the parent directory of the desired path of the archive entry at the destination' {

'Should not attempt to find....'

---

Tests/Unit/MSFT_xArchive.Tests.ps1, line 4370 at r3 (raw file):

                }

                It 'Should find the parent directory of the desired path of the archive entry at the destination' {

'Should not attempt to find...'

---

Tests/Unit/MSFT_xArchive.Tests.ps1, line 4589 at r3 (raw file):

                }

                It 'Should find the parent directory of the desired path of the archive entry at the destination' {

'Should not attempt to find...'

---

Tests/Unit/MSFT_xArchive.Tests.ps1, line 4707 at r3 (raw file):

                }

                It 'Should find the parent directory of the desired path of the archive entry at the destination' {

'Should not attempt to find...'

---

Comments from Reviewable

[mbreakey3]

Reviewed 13 of 14 files at r3.
Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

Comments from Reviewable

[kwirkykat]

Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

Tests/Unit/MSFT_xArchive.Tests.ps1, line 933 at r3 (raw file):

Previously, mbreakey3 (Mariah) wrote…

Is the '*' here supposed to be?

Done.

---

Tests/Unit/MSFT_xArchive.Tests.ps1, line 1837 at r3 (raw file):

Previously, mbreakey3 (Mariah) wrote…

I would add a test for the Checksum parameter name being shorter than 3 chars as well.

Done.

---

Tests/Unit/MSFT_xArchive.Tests.ps1, line 3438 at r3 (raw file):

Previously, mbreakey3 (Mariah) wrote…

'Should set the last access time...'

Done.

---

Tests/Unit/MSFT_xArchive.Tests.ps1, line 3450 at r3 (raw file):

Previously, mbreakey3 (Mariah) wrote…

'Should set the creation time...'

Done.

---

Tests/Unit/MSFT_xArchive.Tests.ps1, line 4248 at r3 (raw file):

Previously, mbreakey3 (Mariah) wrote…

'Should not attempt to find....'

Done.

---

Tests/Unit/MSFT_xArchive.Tests.ps1, line 4370 at r3 (raw file):

Previously, mbreakey3 (Mariah) wrote…

'Should not attempt to find...'

Done.

---

Tests/Unit/MSFT_xArchive.Tests.ps1, line 4589 at r3 (raw file):

Previously, mbreakey3 (Mariah) wrote…

'Should not attempt to find...'

Done.

---

Tests/Unit/MSFT_xArchive.Tests.ps1, line 4707 at r3 (raw file):

Previously, mbreakey3 (Mariah) wrote…

'Should not attempt to find...'

Done.

---

Comments from Reviewable

[mbreakey3]

---

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable