Add PSxUnit runner by chunqingchen · Pull Request #2574 · PowerShell/PowerShell

chunqingchen · Nov 1, 2016

We need a runner that runs across platforms and compatible with .net core
Some of the code are from dotnet-test-xunit as it supports .net core and is a xunit runner, I modify the file to compatible running PS tests and add classification

msftclas · Nov 1, 2016

Hi @chunqingchen, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

daxian-dbw

Hi @chunqingchen, there are a few issues with your PR:

You need to sign the CLA agreement
Your first 2 commits show changes that are already merged: 0908547. And your third commit is a merge commit. Please rebase your working branch and push again.
Please fix the Travis CI failure.

SteveL-MSFT · Nov 1, 2016

@chunqingchen you need to join the Microsoft GitHub org

chunqingchen · Dec 2, 2016

@daxian-dbw
cla signed
redundant commits removed
all checks passed.

adityapatwardhan · Dec 13, 2016

assemblyFilename [](start = 50, length = 16)

Check for null or empty string.

adityapatwardhan · Dec 13, 2016

The same Path.Combine() is done twice. It should be something like:

var runnerJson = Path.Combine(pathWithAssemblyName, $"{assemblyName}.xunit.runner.json")

if(File.Exists(runnerJson)
{
return File.OpenRead(runnerJson);
}

adityapatwardhan · Dec 13, 2016

File.OpenRead [](start = 27, length = 13)

This can throw exceptions. We should catch and fail gracefully.

corrected. Although I don't try to catch most of the exceptions and want to throw them as they are

adityapatwardhan · Dec 13, 2016

Add ///

style comments for all public types and properties.

adityapatwardhan · Dec 13, 2016

//Console.ReadKey(); [](start = 12, length = 20)

Remove unnecessary commented lines.

adityapatwardhan · Dec 13, 2016

use named parameters.

adityapatwardhan · Dec 15, 2016

Maybe this can be moved before the block which checks for "Skip". So if the testcase does not match the filter then we dont need to check if it is skipped.

adityapatwardhan · Dec 15, 2016

(".\result.xml" [](start = 44, length = 16)

Result file name should not be hardcoded but be accepted as a parameter.

adityapatwardhan · Dec 15, 2016

Is this intentional?

adityapatwardhan · Dec 15, 2016

Is this intentional?

Uncommented.

adityapatwardhan · Dec 15, 2016

Mix use of tabs and spaces.

adityapatwardhan · Dec 15, 2016

Please break this into smaller functions. Some code might be reusable and it will become much more readable.

this is actually from the dotnet-test-xunit project so i would just leave it as is

adityapatwardhan · Dec 15, 2016

🕐

chunqingchen · Jan 4, 2017

@adityapatwardhan the comments are resolved.

chunqingchen · Feb 1, 2017

@lzybkr Hi Jason, Aditya has approved the comments, can this be merged?

comment addressed

lzybkr · Feb 13, 2017

Reassigned to @daxian-dbw - he'll want to look at this as part of moving to msbuild based dotnet cli.

chunqingchen · Feb 24, 2017

@daxian-dbw Hi Dongbo, Aditya has approved the comments, can this be merged?

daxian-dbw · Feb 24, 2017

@chunqingchen Sorry that I didn't get a chance to quickly go through this PR. Will do it.

daxian-dbw · Feb 25, 2017

@SteveL-MSFT This PR contains code from XUnit. I just want to double check, is there a concern about checking in third party source code to our repository?

SteveL-MSFT · Feb 25, 2017

@daxian-dbw yes, we need to adhere to their licensing

SteveL-MSFT · Feb 25, 2017

@chunqingchen it looks like we're just taking some of their code without modification? can we have this be a runtime dependency so that xunit is already available?

daxian-dbw · Apr 11, 2017

I'm closing this PR for following 2 reasons:

After moving to netcoreapp2.0, we will refactor our code to get rid of powershell assembly load context, and thus we won't need a special runner to host powershell.
We have moved to MSBuild, and this PR needs to be re-worked to be built with MSBuild.

msftclas added the cla-required label Nov 1, 2016

daxian-dbw previously requested changes Nov 1, 2016

View reviewed changes

PowerShellTeam added the Review - Needed label Nov 10, 2016

HemantMahawar removed the Review-Needed label Dec 1, 2016

chunqingchen added cla-already-signed and removed cla-required labels Dec 2, 2016

lzybkr requested a review from adityapatwardhan December 8, 2016 21:48

lzybkr self-assigned this Dec 8, 2016

adityapatwardhan reviewed Dec 13, 2016

View reviewed changes

daxian-dbw mentioned this pull request Dec 13, 2016

Invoke-RestMethod cmdlet error if the response is neither JSON nor XML #2862

Merged

chunqingchen added 2 commits December 14, 2016 18:50

Add PSxUnit runner

0bca2d8

remove readme file

f2b4a33

adityapatwardhan reviewed Dec 15, 2016

View reviewed changes

Comment thread test/tools/PSxUnit/ConsoleRunnerLogger.cs Outdated

adityapatwardhan Dec 15, 2016

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

Is this intentional?

adityapatwardhan reviewed Dec 15, 2016

View reviewed changes

chunqingchen added 2 commits January 3, 2017 17:18

remove readme file

6dcb5d0

Merge branch 'master' of https://github.com/chunqingchen/PowerShell

64f3a54

Resolve code review for Add PsxUnit.

bfec1ad

adityapatwardhan approved these changes Jan 31, 2017

View reviewed changes

lzybkr assigned daxian-dbw and unassigned lzybkr Feb 13, 2017

daxian-dbw closed this Apr 11, 2017

iSazonov removed the Review - Needed The PR is being reviewed label Apr 20, 2017

Search code, repositories, users, issues, pull requests...

Conversation

chunqingchen commented Nov 1, 2016

Uh oh!

msftclas commented Nov 1, 2016

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Nov 1, 2016

Uh oh!

chunqingchen commented Dec 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adityapatwardhan commented Dec 15, 2016

Uh oh!

chunqingchen commented Jan 4, 2017

Uh oh!

chunqingchen commented Feb 1, 2017

Uh oh!

lzybkr commented Feb 13, 2017

Uh oh!

chunqingchen commented Feb 24, 2017

Uh oh!

daxian-dbw commented Feb 24, 2017

Uh oh!

daxian-dbw commented Feb 25, 2017

Uh oh!

SteveL-MSFT commented Feb 25, 2017

Uh oh!

SteveL-MSFT commented Feb 25, 2017

Uh oh!

daxian-dbw commented Apr 11, 2017

Uh oh!

Reviewers