Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xExchInstall: Add remaining xExchInstall Unit Tests and add common setup tests #321

Merged
merged 6 commits into from
Sep 28, 2018
Merged

xExchInstall: Add remaining xExchInstall Unit Tests and add common setup tests #321

merged 6 commits into from
Sep 28, 2018

Conversation

mhendric
Copy link
Contributor

@mhendric mhendric commented Sep 26, 2018

Pull Request (PR) description

Adds remaining unit tests for xExchInstall, and adds most remaining unit tests for common setup functions

Task list

  • Added an entry under the Unreleased section of the change log in the CHANGELOG.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

@mhendric mhendric mentioned this pull request Sep 26, 2018
9 tasks
@mhendric mhendric added the needs review The pull request needs a code review. label Sep 26, 2018
@codecov-io
Copy link

codecov-io commented Sep 26, 2018

Codecov Report

❗ No coverage uploaded for pull request base (dev@e08c1bc). Click here to learn what that means.
The diff coverage is 54.34%.

Impacted file tree graph

@@          Coverage Diff          @@
##             dev    #321   +/-   ##
=====================================
  Coverage       ?   5.82%           
=====================================
  Files          ?      38           
  Lines          ?    4448           
  Branches       ?       0           
=====================================
  Hits           ?     259           
  Misses         ?    4189           
  Partials       ?       0
Impacted Files Coverage Δ
...Resources/MSFT_xExchInstall/MSFT_xExchInstall.psm1 100% <100%> (ø)
Modules/xExchangeHelper.psm1 37.4% <100%> (ø)
...FT_xExchEventLogLevel/MSFT_xExchEventLogLevel.psm1 4.54% <4.54%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e08c1bc...ca81a63. Read the comment docs.

Copy link
Member

@johlju johlju left a comment

Choose a reason for hiding this comment

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

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


Tests/Unit/MSFT_xExchInstall.tests.ps1, line 58 at r2 (raw file):

        }

        Describe 'MSFT_xExchInstall\Set-TargetResource' -Tag 'Set' {

Most of the tests below do not assert that the mocks was called. I think we should assert that they are called (or called the correct number of times).


Tests/Unit/xExchangeHelper.tests.ps1, line 615 at r2 (raw file):

'Should do nothing'

Maybe: 'Should not configure WinRM'


Tests/Unit/xExchangeHelper.tests.ps1, line 774 at r2 (raw file):

'Should do nothing'

Maybe revise this to explain what exactly it should not do?

@johlju johlju 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 Sep 27, 2018
@johlju
Copy link
Member

johlju commented Sep 27, 2018

@mhendric Can you verify that the new line endings didn't break the Excel files, e.g E2013Calcv6.6 - Lab.xlsm? If they did, then maybe we need to exclude those file types.

@mhendric
Copy link
Contributor Author

mhendric commented Sep 27, 2018

@johlju It looks like this does break the Excel files. I'll go ahead and revert those to their previous state. Sounds like we will need to exclude these in the future though. What's the best way to do that?

@mhendric
Copy link
Contributor Author

@johlju I think I figured out how to exclude Excel files. See the included modifications to .gitattributes. Aside from that, all your suggested changes have been made. 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 Sep 27, 2018
Copy link
Member

@johlju johlju 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 3 of 6 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@johlju 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 Sep 28, 2018
@mhendric mhendric merged commit 1fd2670 into dsccommunity:dev Sep 28, 2018
@johlju johlju removed the ready for merge The pull request was approved by the community and is ready to be merged by a maintainer. label Sep 28, 2018
@mhendric mhendric deleted the AddCommonSetupTests branch February 7, 2019 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants