Closed ld0614 closed 6 years ago
Thought I would help out and review this one for you. I did not do any style review comments on this since I don't know if you are there yet. Let me know if you want me to do that.
Reviewed 13 of 13 files at r1. Review status: all files reviewed at latest revision, 3 unresolved discussions.
appveyor.yml, line 1 at r1 (raw file):
#---------------------------------#
Non-blocking. You could simplify this by using the default model of the test framework, which do all this, see example here: https://github.com/PowerShell/xComputerManagement/blob/dev/appveyor.yml and https://github.com/PowerShell/xSQLServer/blob/dev/appveyor.yml (same as xComputerManagement, but do some initial configuration on the build worker)
README.md, line 3 at r1 (raw file):
# xRemoteDesktopSessionHost [![Build status](https://ci.appveyor.com/api/projects/status/ly6w6vaavkshrpg8/branch/master?svg=true)](https://ci.appveyor.com/project/PowerShell/xremotedesktopsessionhost/branch/master)
Non-blocking, You could use the Branches section from the other repos, see xComputerManagement as an example. If you resolve the above comment, then you can add CodeCov badge as well. https://github.com/PowerShell/xComputerManagement/blob/dev/README.md
tests/unit/xRemoteDesktopSessionHost.tests.ps1, line 26 at r1 (raw file):
} function Invoke-TestCleanup {
Missing Restore-TestEnvironment -TestEnvironment $TestEnvironment
here. Is there a reason that we could not have it in this test?
Comments from Reviewable
:exclamation: No coverage uploaded for pull request base (
dev@6122cd1
). Click here to learn what that means. The diff coverage is0%
.
@@ Coverage Diff @@
## dev #29 +/- ##
=====================================
Coverage ? 16.1%
=====================================
Files ? 5
Lines ? 118
Branches ? 4
=====================================
Hits ? 19
Misses ? 95
Partials ? 4
Impacted Files | Coverage Δ | |
---|---|---|
...ration/MSFT_xRDSessionCollectionConfiguration.psm1 | 9.09% <0%> (ø) |
|
...RDSessionDeployment/MSFT_xRDSessionDeployment.psm1 | 20% <0%> (ø) |
|
...RDSessionCollection/MSFT_xRDSessionCollection.psm1 | 21.05% <0%> (ø) |
|
...Resources/MSFT_xRDRemoteApp/MSFT_xRDRemoteApp.psm1 | 11.11% <0%> (ø) |
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 6122cd1...8388c2a. Read the comment docs.
Thanks @johlju for the review, I've not completed a style update yet, I think the best thing to do is run that as a separate PR due to the number of updates involved. Happy to take direction and keep this PR open if that's the better approach though
Review status: 11 of 13 files reviewed at latest revision, 3 unresolved discussions, some commit checks broke.
appveyor.yml, line 1 at r1 (raw file):
Non-blocking. You could simplify this by using the default model of the test framework, which do all this, see example here: https://github.com/PowerShell/xComputerManagement/blob/dev/appveyor.yml and https://github.com/PowerShell/xSQLServer/blob/dev/appveyor.yml (same as xComputerManagement, but do some initial configuration on the build worker)
Done.
README.md, line 3 at r1 (raw file):
Non-blocking, You could use the Branches section from the other repos, see xComputerManagement as an example. If you resolve the above comment, then you can add CodeCov badge as well. https://github.com/PowerShell/xComputerManagement/blob/dev/README.md
Done.
tests/unit/xRemoteDesktopSessionHost.tests.ps1, line 26 at r1 (raw file):
Missing `Restore-TestEnvironment -TestEnvironment $TestEnvironment`here. Is there a reason that we could not have it in this test?
Yes, this script doesn't actually run Initialize-TestEnvironment and therefore the restore-TestEnvironment fails. The tests themselves need a significant amount of work on them but I was going to keep that as a separate piece of work due to the amount of work required.
Comments from Reviewable
No problem, happy to help. :smile: You are welcome to tag me if you want a second pair of eyes on a PR.
I agree it's better to do separate PR's for the style and other improvements.
Reviewed 2 of 2 files at r2. Review status: all files reviewed at latest revision, all discussions resolved, some commit checks broke.
Comments from Reviewable
Thanks @johlju What type of merge should this be done under? I also assume that I can merge despite codecov failing as this is the first time it is being enabled?
It’s up to you and maybe dependent on circumstances. Also depends if you want to retain the working branch commit history. I usually use “Squash commit” Which squashed all the working branch commits (PR commits) into a new commit. As commit title I normally use PR title, and for the commit message I normally use the text in the change log from the PR.
There are a number of issues with the current module when run through the standard battery of tests in Appveyor. This PR goes through and fixes these. They are all very minor, mainly formatting and Parameter changes and there shouldn't be any functional impact to the module.
I have also updated the copyright information as per the guidelines and fixed the failing unit tests while bringing them in line with the latest testing template.
This change is