GeezerVison

Chunqing Chen (chunqingchen) force-pushed the xunitTest branch from d0b8a16 to db12347 Compare

September 8, 2017 00:26

Dongbo Wang (daxian-dbw) requested review from James Truher (JamesWTruher), Steve Lee (SteveL-MSFT) and Andrew (anmenaga)

September 8, 2017 04:46

Mark Kraus (markekraus) reviewed

View reviewed changes

Comment thread

build.psm1 Outdated

Mark Kraus (markekraus) Sep 8, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This should probably stay before any calls made to dotnet.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

TravisEz13 and adityapatwardhan reacted with thumbs up emoji

All reactions

👍 2 reactions

Chunqing Chen (chunqingchen) Sep 18, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

resolved

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

markekraus reacted with thumbs up emoji

All reactions

👍 1 reaction

James Truher (JamesWTruher) suggested changes

View reviewed changes

James Truher (JamesWTruher) left a comment

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

a couple of the tests aren't testing the thing they purport to test, so they should be fixed or removed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/csharp/test_ExtensionMethods.cs Outdated

James Truher (JamesWTruher) Sep 8, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This doesn't seem to be checking the method on the PSTypeExtensions object. Shouldn't this be something like:
Assert.False(PSTypeExtensions.IsNumeric("string".GetType())
It should actually test the PSTypeExtensions object or just removed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Chunqing Chen (chunqingchen) Sep 18, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

resolved.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/csharp/test_Binders.cs Outdated

James Truher (JamesWTruher) Sep 8, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

see my comment below

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Chunqing Chen (chunqingchen) Sep 18, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

resolved

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/csharp/test_CorePsPlatform.cs Outdated

James Truher (JamesWTruher) Sep 8, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

this isn't very useful and no longer tests what it is reported to test. This needs to check to see that the username returned by whoami is the same as Platform.Unix.UserName. Also, this is a non-windows only test. Is there a way to notate that?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

TravisEz13 reacted with thumbs up emoji

All reactions

👍 1 reaction

Chunqing Chen (chunqingchen) Sep 19, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

resolved

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dongbo Wang (daxian-dbw) assigned Aditya Patwardhan (adityapatwardhan)

Chunqing Chen (chunqingchen) force-pushed the xunitTest branch from db12347 to 08d5cde Compare

September 19, 2017 00:55

Travis Plunk (TravisEz13) commented Sep 19, 2017

Copy link

Copy Markdown

Member

James Truher (@JamesWTruher) Can you update your review?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Microsoft GitHub User (msftgits) removed the cla-not-required label

Sep 26, 2017

Aditya Patwardhan (adityapatwardhan) requested changes

Sep 26, 2017

View reviewed changes

Comment thread

build.psm1 Outdated

Aditya Patwardhan (adityapatwardhan) Sep 26, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

How is a failure in xUnit propagated to a failure in AppVeyor / Travis?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

James Truher (JamesWTruher) reviewed

Sep 26, 2017

View reviewed changes

James Truher (JamesWTruher) left a comment

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

if everything actually runs on MacOS/Linux ok then I feel it would be better to get it in sooner rather than later. However, Aditya's question needs to be answered, how do we incorporate these results into our pester results?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/csharp/test_CorePsPlatform.cs Outdated

James Truher (JamesWTruher) Sep 26, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

i believe that this is still not good for MacOS. hostname --fqdn is an error. Will this run on Mac?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Aditya Patwardhan (adityapatwardhan) force-pushed the xunitTest branch from 08d5cde to fac8815 Compare

October 25, 2017 20:54

Chunqing Chen (chunqingchen) closed this

Oct 30, 2017

Aditya Patwardhan (adityapatwardhan) commented Oct 30, 2017

Copy link

Copy Markdown

Member

Chunqing Chen (@chunqingchen) This needs to be open. I am working on it.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Aditya Patwardhan (adityapatwardhan) reopened this

Oct 30, 2017

Aditya Patwardhan (adityapatwardhan) force-pushed the xunitTest branch from cd552f4 to 91b85a8 Compare

October 30, 2017 23:11

Aditya Patwardhan (adityapatwardhan) force-pushed the xunitTest branch 2 times, most recently from bb3966b to d449465 Compare

November 28, 2017 01:08

Aditya Patwardhan (adityapatwardhan) approved these changes

View reviewed changes

Aditya Patwardhan (adityapatwardhan) commented Nov 28, 2017

Copy link

Copy Markdown

Member

James Truher (@JamesWTruher) Dongbo Wang (@daxian-dbw) I have updated the PR with multiple changes list in the PR description. All tests pass in CI and the log file is uploaded to artifacts on AppVeyor. Please have look.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Aditya Patwardhan (adityapatwardhan) assigned Dongbo Wang (daxian-dbw) and unassigned Aditya Patwardhan (adityapatwardhan)

James Truher (JamesWTruher) reviewed

View reviewed changes

Comment thread

build.psm1 Outdated

James Truher (JamesWTruher) Nov 28, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

this should probably be throw or something - if $results has a problem then it might look like everything passed since the error will likely be hidden in the log

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Aditya Patwardhan (adityapatwardhan) Nov 28, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

added throw instead of Write-Error

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

build.psm1 Outdated

James Truher (JamesWTruher) Nov 28, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

can this be simplified to just:
if ( -not (test-path (get-psoutput)))

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Aditya Patwardhan (adityapatwardhan) Nov 28, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We use $Content later. So I will keep it as is.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/csharp/test_MshSnapinInfo.cs Outdated

James Truher (JamesWTruher) Nov 28, 2017

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

does this need a Skip.IfNot(Platform.IsWindows)?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Aditya Patwardhan (adityapatwardhan) Nov 28, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Added

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

James Truher (JamesWTruher) approved these changes

View reviewed changes

Dongbo Wang (daxian-dbw) approved these changes

Nov 30, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Leave some comments

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

build.psm1 Outdated

Dongbo Wang (daxian-dbw) Nov 30, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

maybe the single quote is not needed ...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Aditya Patwardhan (adityapatwardhan) Nov 30, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

build.psm1 Outdated

Dongbo Wang (daxian-dbw) Nov 30, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

maybe move this line closer to its uses.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/csharp/csharp.tests.csproj Outdated

Dongbo Wang (daxian-dbw) Nov 30, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Maybe not needed as it's already included in Tests.Common.props.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/csharp/csharp.tests.csproj Outdated

Dongbo Wang (daxian-dbw) Nov 30, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It's not just Linux now.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

test/csharp/csharp.tests.csproj Outdated

Dongbo Wang (daxian-dbw) Nov 30, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please add a comment here

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

tools/appveyor.psm1 Outdated

Dongbo Wang (daxian-dbw) Nov 30, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

it will write a true or false in console, should be suppressed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

tools/travis.ps1 Outdated

Dongbo Wang (daxian-dbw) Nov 30, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

nit: the ending quote is not needed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Aditya Patwardhan (adityapatwardhan) commented Nov 30, 2017

Copy link

Copy Markdown

Member

Dongbo Wang (@daxian-dbw) All comments have been addressed and all tests have passed.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Aditya Patwardhan (adityapatwardhan) commented Nov 30, 2017

Copy link

Copy Markdown

Member

Dongbo Wang (@daxian-dbw) is this ready to merge?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Aditya Patwardhan (adityapatwardhan) added 5 commits

November 30, 2017 14:15


          Initial work to enable xunit

473a39c


          Moved AssemblyOriginatorKeyFile to csharp.tests.csproj

73ac2ca


          Native binary has dylib extension on macOS

111b68d


          Addressed CR comments

15f68e9


          Addressed CR comments

d7b7433

Aditya Patwardhan (adityapatwardhan) force-pushed the xunitTest branch from faa4ee7 to d7b7433 Compare

November 30, 2017 22:16

Aditya Patwardhan (adityapatwardhan) merged commit 52df947 into PowerShell:master

Nov 30, 2017

Travis Plunk (TravisEz13) modified the milestone: 6.0.0-GA

Dec 1, 2017

Dongbo Wang (daxian-dbw) mentioned this pull request

Dec 1, 2017

Enable xUnit tests on OS X #851

Closed

Travis Plunk (TravisEz13) added this to the 6.0.0-RC.2 milestone

Travis Plunk (TravisEz13) pushed a commit to TravisEz13/PowerShell that referenced this pull request


          Fix xunnit test for PS (PowerShell#4780)

1ed5e7f

* Initial work to enable xunit

* Moved AssemblyOriginatorKeyFile to csharp.tests.csproj

* Native binary has dylib extension on macOS

Travis Plunk (TravisEz13) mentioned this pull request

Merge 6.0.0 rc.2 changes #5606

Merged

Travis Plunk (TravisEz13) pushed a commit that referenced this pull request