Skip to content
This repository has been archived by the owner on Jun 14, 2024. It is now read-only.

Fix msi package tests #143

Merged
merged 8 commits into from
May 24, 2019
Merged

Fix msi package tests #143

merged 8 commits into from
May 24, 2019

Conversation

mhendric
Copy link
Contributor

@mhendric mhendric commented Apr 23, 2019

Pull Request (PR) description

Fixes issue where MsiPackage Integration tests fail if strong crypto is not enabled for .NET 4.

This Pull Request (PR) fixes the following issues

Task list

  • Added an entry under the Unreleased section of the change log in the README.md.
    Entry should say what was changed, and how that affects users (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 Resource Testing Guidelines.
  • Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
  • New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

This change is Reviewable

@codecov-io
Copy link

codecov-io commented Apr 23, 2019

Codecov Report

Merging #143 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@         Coverage Diff         @@
##            dev   #143   +/-   ##
===================================
  Coverage    83%    83%           
===================================
  Files        19     19           
  Lines      2760   2760           
  Branches      4      4           
===================================
  Hits       2305   2305           
  Misses      451    451           
  Partials      4      4

@mhendric
Copy link
Contributor Author

@PlagueHO , would you mind giving this one a review? Thanks.

@mhendric mhendric requested a review from PlagueHO April 23, 2019 01:12
@mhendric mhendric added the needs review The pull request needs a code review. label Apr 23, 2019
@PlagueHO
Copy link
Contributor

Sorry about the delay @mhendric ! Back on board now (been tied up with other projects the last few weeks). Reviewing now!

@PlagueHO
Copy link
Contributor

@mhendric - needs conflicts resolved BTW.

Copy link
Contributor

@PlagueHO PlagueHO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mhendric)


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

* Fixes issue where MsiPackage Integration tests fail to make an HTTPS
  connection if Strong Crypto for .NET is not enabled.

Minor nitpick: for consistency could use: ...NET is not enabled - Fixes Issue #142.

Although it doesn't look like there is huge consistency anyway 😁 so up to you.


Tests/Integration/MSFT_MsiPackage.EndToEnd.Tests.ps1, line 23 at r1 (raw file):

# Make sure strong crypto is enabled in .NET for HTTPS tests
Enable-StrongCryptoForDotNetFour

Should we also preserve state and restore it afterwards? If tests are being run on a developer machine it might cause difficult to diagnose issues.


Tests/Integration/MSFT_MsiPackage.Integration.Tests.ps1, line 21 at r1 (raw file):

# Make sure strong crypto is enabled in .NET for HTTPS tests
Enable-StrongCryptoForDotNetFour

Should we also preserve state and restore it afterwards? If tests are being run on a developer machine it might cause difficult to diagnose issues.


Tests/TestHelpers/CommonTestHelper.psm1, line 800 at r1 (raw file):

{
    [CmdletBinding()]
    param()

Can you add space after param?


Tests/TestHelpers/CommonTestHelper.psm1, line 826 at r1 (raw file):

        else
        {
            Write-Warning -Message "Failed to find registry key at path: $regPath. Skipping setting SchUseStrongCrypto to 1."

Maybe mention that this may cause Integration tests to fail in the message here?

@PlagueHO
Copy link
Contributor

PlagueHO commented May 3, 2019

Let me know when you're ready for me to finish the review @mhendric

@mhendric
Copy link
Contributor Author

mhendric commented May 6, 2019

Will do @PlagueHO . Sorry for the delay. I haven't had a chance to troubleshoot the most recent test failures yet. I'll ping you once I get a clean build though. Thanks.

@mhendric mhendric added waiting for code fix A review left open comments, and the pull request is waiting for changes to be pushed by the author. and removed needs review The pull request needs a code review. labels May 6, 2019
@mhendric
Copy link
Contributor Author

mhendric commented May 9, 2019

Hey @PlagueHO , finally got this fixed. Would you mind reviewing again when you have a chance? Thanks.

@mhendric mhendric 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 May 9, 2019
@mhendric
Copy link
Contributor Author

Hi @PlagueHO , checking to see if you can complete the review on this one? This PR is holding up a couple other PR's. Thanks.

@PlagueHO
Copy link
Contributor

Oh yes. Sorry about that @mhendric - I'll aim to complete tonight.

Copy link
Contributor

@PlagueHO PlagueHO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 4 files at r2, 2 of 2 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mhendric)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs review The pull request needs a code review.
Projects
None yet
3 participants