RTA: xWebApplication Integration tests & fixes #136

nzspambot · 2016-05-23T09:10:50Z

Hi, added some basic xWebApplication integration tests as well as some fixes to xWebApplication

This change is

… and unit test

msftclas · 2016-05-23T09:10:54Z

Hi @nzspambot, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

nzspambot · 2016-05-23T09:26:26Z

appveyor failed due to this btw

Exception calling "UploadFile" with "2" argument(s): "The remote server returned an error: (500) Internal Server Error."
At line:3 char:97
+ ... /ci.appveyor.com/api/testresults/nunit/$($env:APPVEYOR_JOB_ID)", (Res ...
+                                              ~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [], MethodInvocationException
    + FullyQualifiedErrorId : WebException

Command executed with exception: Exception calling "UploadFile" with "2" argument(s): "The remote server returned an error: (500) Internal Server Error."

PlagueHO · 2016-05-25T08:40:51Z

I'll give this a review tomorrow night unless someone else gets to it first. I'll also take a look at the AppVeyor issue.

nzspambot · 2016-05-25T09:03:33Z

@PlagueHO I'm assuming you're referring to the Integration git errors? Appveyor is working ok now

nzspambot · 2016-05-25T10:39:14Z

I think appveyor has hung on this latest build

nzspambot · 2016-05-26T10:09:09Z

@PlagueHO do you have access to rerun a appveyor build?

tysonjhayes · 2016-05-26T18:30:46Z

Unfortunately neither @PlagueHO or I have rights to rerun it. Easiest way is to add a line break somewhere and checkin which kicks off another build (terrible I know) or page @KarolKaczmarek or @TravisEz13 and ask nicely. 😁

nzspambot · 2016-05-26T22:46:52Z

All good; pushed some style changes so see if this builds now 🏁

…ate Version: 1.1.0 region, changed double quotes into single quotes where needed

PlagueHO · 2016-05-27T23:13:22Z

Reviewed 4 of 5 files at r1, 2 of 3 files at r3, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

README.md, line 192 [r5] (raw file):

* Adding conditional logic to install the test helper module from the gallery if the user downloaded the module from the gallery.
* Added **xWebApplication** integration tests
* Added fixes to **xWebApplication**

Can we mention what the fixes to xWebApplication were? New SSL Flags added etc

DSCResources/MSFT_xWebApplication/MSFT_xWebApplication.schema.mof, line 0 [r5] (raw file):
There appears to be a problem with the MSFT_xSSLSettings.schema.mof
Both Reviewable and GitHub are reporting this file is a binary file (so the content is not being shown for review). Can you check it please? Might need to recreate it or check the char encoding etc.

Tests/Integration/MSFT_xWebApplication.Integration.Tests.ps1, line 4 [r5] (raw file):

$Global:DSCResourceName = 'MSFT_xWebApplication'

#region HEADER

Could you update the header here or add the version number?

Nice work on all of this - especially the integration tests too. Nice to see such complete coverage, I think everyone should be trying to achieve this level (including myself)! 😄

Tests/Integration/MSFT_xWebAppPoolDefaults.Integration.Tests.ps1, line 4 [r5] (raw file):

$Global:DSCResourceName    = 'MSFT_xWebAppPoolDefaults'

# Integration Test Template Version: 1.1.0

Can you add the #region HEADER above this line (just so the #Region collapse still works).

Comments from Reviewable

nzspambot · 2016-05-28T01:31:03Z

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

README.md, line 192 [r5] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can we mention what the fixes to xWebApplication were? New SSL Flags added etc

Done.

Tests/Integration/MSFT_xWebApplication.Integration.Tests.ps1, line 4 [r5] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Could you update the header here or add the version number?

Nice work on all of this - especially the integration tests too. Nice to see such complete coverage, I think everyone should be trying to achieve this level (including myself)! 😄

Done.

Tests/Integration/MSFT_xWebAppPoolDefaults.Integration.Tests.ps1, line 4 [r5] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Can you add the #region HEADER above this line (just so the #Region collapse still works).

Done.

Comments from Reviewable

nzspambot · 2016-05-28T01:37:22Z

@PlagueHO ok updated 👍

nzspambot · 2016-05-28T01:50:12Z

hmm appveyor failed yet again

Exception calling "UploadFile" with "2" argument(s): "The remote server returned an error: (500) Internal Server Error."
At line:3 char:97
+ ... /ci.appveyor.com/api/testresults/nunit/$($env:APPVEYOR_JOB_ID)", (Res ...
+                                              ~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [], MethodInvocationException
    + FullyQualifiedErrorId : WebException

Command executed with exception: Exception calling "UploadFile" with "2" argument(s): "The remote server returned an error: (500) Internal Server Error."

I reckon this could be wrapped in a try/catch/while loop

e: appveyor/ci#826 :tumbleweeds:

PlagueHO · 2016-05-28T04:18:31Z

That is actually a pretty good idea. If we do this perhaps we do it to the template one and advise it to be changed for all AppVeyor.yml (I've run into the same thing in lots of other projects). Perhaps we could use xWebAdministration as the test project for this change - @tysonjhayes?

Anyway, @tysonjhayes, this so ready for merge.

Previously, nzspambot (James Baker) wrote…

hmm appveyor failed yet again


Exception calling "UploadFile" with "2" argument(s): "The remote server returned an error: (500) Internal Server Error."

At line:3 char:97

+ ... /ci.appveyor.com/api/testresults/nunit/$($env:APPVEYOR_JOB_ID)", (Res ...

+                                              ~~~~~~~~~~~~~~~~~~~~

    + CategoryInfo          : NotSpecified: (:) [], MethodInvocationException

    + FullyQualifiedErrorId : WebException



Command executed with exception: Exception calling "UploadFile" with "2" argument(s): "The remote server returned an error: (500) Internal Server Error."

I reckon this could be wrapped in a try/catch/while loop

e: appveyor/ci#826 :tumbleweeds:

Reviewed 3 of 3 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

README.md, line 192 [r5] (raw file):

Previously, nzspambot (James Baker) wrote…

Done.

Thanks sir!

Comments from Reviewable

PlagueHO · 2016-05-28T04:19:33Z

Review status: 7 of 8 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

DSCResources/MSFT_xWebApplication/MSFT_xWebApplication.schema.mof, line [r5] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

There appears to be a problem with the MSFT_xSSLSettings.schema.mof
Both Reviewable and GitHub are reporting this file is a binary file (so the content is not being shown for review). Can you check it please? Might need to recreate it or check the char encoding etc.

Actually, I still missed this issue. Did you manage to figure out what is wrong with this MOF File?

Comments from Reviewable

nzspambot · 2016-05-28T05:19:46Z

Yep it was UTF16 👎 fixed and build is running now

OOI (before I dig around) is there a check in the Test resource to check for this?

Could be a nice addition to flag things like this; I know it checks for ascii

PlagueHO · 2016-05-28T05:59:09Z

There is supposed to be:

https://github.com/PowerShell/DscResource.Tests/blob/dev/Meta.Tests.ps1#L99
https://github.com/PowerShell/DscResource.Tests/blob/dev/MetaFixers.psm1#L67

I wonder if these tests could be updated to fail the file?

nzspambot · 2016-05-28T06:09:08Z

Yeah I'd say fail is better IMO; warning can get lost

e: actually it will fail : $unicodeFilesCount | Should Be 0

interesting I might test that out a bit further then

PlagueHO · 2016-05-28T06:33:02Z

It should fail - but for some reason in this case it didn't. Needs to be fixed I reckon!

TravisEz13 · 2016-05-31T01:20:04Z

Review status: 7 of 8 files reviewed at latest revision, 3 unresolved discussions.

DSCResources/MSFT_xWebApplication/MSFT_xWebApplication.psm1, line 217 [r6] (raw file):

              Set-ItemProperty -Path "IIS:\Sites\$Website\$Name" `
                               -Name serviceAutoStartEnabled `
                               -Value $serviceAutoStartEnabled `

We should not be replacing spaces with tabs.
see Style guidelines -general, point 1

Comments from Reviewable

nzspambot · 2016-05-31T05:12:52Z

Do we really have to go through a Tab vs Spaces argument?

tysonjhayes · 2016-05-31T18:19:00Z

@nzspambot as annoying as it is I've actually seen bugs with commands auto completing and ruining intended functionallity when a script has tabs in it, vs I do not see this with spaces. So the intent is not to spark a ververant debate but to actually correct for unintentded bugs (imo).

nzspambot · 2016-05-31T21:23:03Z

@tysonjhayes understood; as long as there is a valid reason I'm 👍

e: Especially after the latest episode of Silicon Valley ;) ;)

PlagueHO · 2016-06-01T08:58:35Z

I've run into issues with Tabs and PowerShell as well (especially with back ticks).

As I was reading these reviews I was watching the most recent episode of Silicon Valley - with the Spaces vs. Tabs conflict - made me LOL. If you haven't watched it (or watch this show), I highly recommend it.

TravisEz13 · 2016-06-07T20:27:50Z

Reviewed 1 of 1 files at r7.
Review status: 7 of 8 files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

nzspambot · 2016-06-08T00:16:40Z

Review status: 7 of 8 files reviewed at latest revision, 3 unresolved discussions.

DSCResources/MSFT_xWebApplication/MSFT_xWebApplication.psm1, line 217 [r6] (raw file):

Previously, TravisEz13 (Travis Plunk) wrote…

We should not be replacing spaces with tabs.
see Style guidelines -general, point 1

Done.

Comments from Reviewable

PlagueHO · 2016-06-09T06:15:23Z

Reviewed 1 of 1 files at r8.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

nzspambot · 2016-06-09T06:36:02Z

I removed the tabs as requested

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

I've run into issues with Tabs and PowerShell as well (especially with back ticks).

As I was reading these reviews I was watching the most recent episode of Silicon Valley - with the Spaces vs. Tabs conflict - made me LOL. If you haven't watched it (or watch this show), I highly recommend it.

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

Comments from Reviewable

nzspambot · 2016-06-09T06:36:45Z

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

README.md, line 192 [r5] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Thanks sir!

np

Comments from Reviewable

tysonjhayes · 2016-06-09T20:55:25Z

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

Tests/Integration/MSFT_xWebApplication.Integration.Tests.ps1, line 28 [r8] (raw file):

    . $ConfigFile

    $null = Backup-WebConfiguration -Name $tempName

We should move this above loading the config file (basically the firs tthing we do) as it can present some errors in restoring/removing the backup if the config file ever gets horked.

Tests/Integration/MSFT_xWebApplication.Integration.Tests.ps1, line 57 [r8] (raw file):

    # Create a new website for the WebApplication

    New-Website -Name $DSCConfig.AllNodes.Website `

Does it make sense to do this with a params hashtable? Might look a bit cleaner.

Comments from Reviewable

nzspambot · 2016-06-09T22:57:19Z

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

DSCResources/MSFT_xWebApplication/MSFT_xWebApplication.schema.mof, line [r5] (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Actually, I still missed this issue. Did you manage to figure out what is wrong with this MOF File?

This should be fixed

Tests/Integration/MSFT_xWebApplication.Integration.Tests.ps1, line 28 [r8] (raw file):

Previously, tysonjhayes (Tyson J. Hayes) wrote…

We should move this above loading the config file (basically the firs tthing we do) as it can present some errors in restoring/removing the backup if the config file ever gets horked.

Done.

Tests/Integration/MSFT_xWebApplication.Integration.Tests.ps1, line 57 [r8] (raw file):

Previously, tysonjhayes (Tyson J. Hayes) wrote…

Does it make sense to do this with a params hashtable? Might look a bit cleaner.

It's only ever called once so I think no

Comments from Reviewable

tysonjhayes · 2016-06-11T01:17:24Z

Reviewed 1 of 1 files at r9.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

Comments from Reviewable

PlagueHO · 2016-06-11T01:52:43Z

Previously, nzspambot (James Baker) wrote…

I removed the tabs as requested

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from Reviewable

PlagueHO · 2016-06-11T01:54:09Z

Oh wait - looks like there is still a merge conflict to resolve. 😦

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

:lgtm:

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from Reviewable

PlagueHO · 2016-06-11T01:54:21Z

Reviewed 1 of 1 files at r7, 1 of 1 files at r9.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from Reviewable

Added xWebApplication basic int tests; added fixes to xWebApplication…

e5012ae

… and unit test

msftclas added the cla-already-signed label May 23, 2016

James Baker added 2 commits May 24, 2016 11:25

Backported changes from xSSLSettings

251c2ee

Removed a single whitespace

564c222

This was referenced May 25, 2016

xWebApplication SSLFlags - Results from Get do not match Set/Test parameters #138

Closed

Remove Unnecessary Parameters in xWebApplication #133

Closed

James Baker added 2 commits May 25, 2016 20:13

Added Ssl128 as and option for SslFlags

b9ff984

Updated README

2f5d3ef

Styleguidelines adjustments - most lines are now under 100 chars

6dc70fb

James Baker added 3 commits May 27, 2016 08:56

Styleguidelines backtick fix

9e594fe

Styleguidelines improvments

0b4db88

Fixed failed test in DefaultAppPool; corrected Integration Test Templ…

aa518ff

…ate Version: 1.1.0 region, changed double quotes into single quotes where needed

nzspambot mentioned this pull request May 27, 2016

RTA: xIISLogging resource + logging in xWebsite #132

Merged

Updated README with changes, fixes to Integration tests HEADER

b5aec44

Fixed encoding on schema file

4852cd4

PlagueHO mentioned this pull request May 28, 2016

Text Files Formatting test should fail on UTF16 files PowerShell/DscResource.Tests#61

Open

tabs > spaces

746532a

Updated integration test after review

4bd8bc2

nzspambot mentioned this pull request Jun 9, 2016

PSSA rule violations in MSFT_xWebApplication #149

Closed

tysonjhayes merged commit 4bd8bc2 into dsccommunity:dev Jun 11, 2016

nzspambot deleted the IntegrationTests-xWebApplication branch June 23, 2016 09:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RTA: xWebApplication Integration tests & fixes #136

RTA: xWebApplication Integration tests & fixes #136

nzspambot commented May 23, 2016 •

edited

Loading

msftclas commented May 23, 2016

nzspambot commented May 23, 2016 •

edited

Loading

PlagueHO commented May 25, 2016

nzspambot commented May 25, 2016 •

edited

Loading

nzspambot commented May 25, 2016

nzspambot commented May 26, 2016

tysonjhayes commented May 26, 2016

nzspambot commented May 26, 2016

PlagueHO commented May 27, 2016

nzspambot commented May 28, 2016

nzspambot commented May 28, 2016

nzspambot commented May 28, 2016 •

edited

Loading

PlagueHO commented May 28, 2016

PlagueHO commented May 28, 2016

nzspambot commented May 28, 2016

PlagueHO commented May 28, 2016

nzspambot commented May 28, 2016 •

edited

Loading

PlagueHO commented May 28, 2016

TravisEz13 commented May 31, 2016

nzspambot commented May 31, 2016

tysonjhayes commented May 31, 2016

nzspambot commented May 31, 2016 •

edited

Loading

PlagueHO commented Jun 1, 2016

TravisEz13 commented Jun 7, 2016

nzspambot commented Jun 8, 2016

PlagueHO commented Jun 9, 2016

nzspambot commented Jun 9, 2016

nzspambot commented Jun 9, 2016

tysonjhayes commented Jun 9, 2016

nzspambot commented Jun 9, 2016

tysonjhayes commented Jun 11, 2016

PlagueHO commented Jun 11, 2016

PlagueHO commented Jun 11, 2016

PlagueHO commented Jun 11, 2016

RTA: xWebApplication Integration tests & fixes #136

RTA: xWebApplication Integration tests & fixes #136

Conversation

nzspambot commented May 23, 2016 • edited Loading

msftclas commented May 23, 2016

nzspambot commented May 23, 2016 • edited Loading

PlagueHO commented May 25, 2016

nzspambot commented May 25, 2016 • edited Loading

nzspambot commented May 25, 2016

nzspambot commented May 26, 2016

tysonjhayes commented May 26, 2016

nzspambot commented May 26, 2016

PlagueHO commented May 27, 2016

nzspambot commented May 28, 2016

nzspambot commented May 28, 2016

nzspambot commented May 28, 2016 • edited Loading

PlagueHO commented May 28, 2016

PlagueHO commented May 28, 2016

nzspambot commented May 28, 2016

PlagueHO commented May 28, 2016

nzspambot commented May 28, 2016 • edited Loading

PlagueHO commented May 28, 2016

TravisEz13 commented May 31, 2016

nzspambot commented May 31, 2016

tysonjhayes commented May 31, 2016

nzspambot commented May 31, 2016 • edited Loading

PlagueHO commented Jun 1, 2016

TravisEz13 commented Jun 7, 2016

nzspambot commented Jun 8, 2016

PlagueHO commented Jun 9, 2016

nzspambot commented Jun 9, 2016

nzspambot commented Jun 9, 2016

tysonjhayes commented Jun 9, 2016

nzspambot commented Jun 9, 2016

tysonjhayes commented Jun 11, 2016

PlagueHO commented Jun 11, 2016

PlagueHO commented Jun 11, 2016

PlagueHO commented Jun 11, 2016

nzspambot commented May 23, 2016 •

edited

Loading

nzspambot commented May 23, 2016 •

edited

Loading

nzspambot commented May 25, 2016 •

edited

Loading

nzspambot commented May 28, 2016 •

edited

Loading

nzspambot commented May 28, 2016 •

edited

Loading

nzspambot commented May 31, 2016 •

edited

Loading