Move xUnit tests in new folder by iSazonov · Pull Request #8356 · PowerShell/PowerShell

iSazonov · 2018-11-29T05:11:35Z

(Replace #8237)

Motivation

I have a PR where there are many new xUnit tests.
It would also be useful to create new xUnit tests for public APIs.
The number of xUnit tests will increase and their ordering is required.

PR Summary

Move C# xUnit tests in new folder. This allows to put new xUnit tests in directory structure in accordance with directory structure where cs files are.
Use an xUnit TestCaseOrderer attribute to sequentially process tests for powershell.config.json.
Update README.md
A race condition was fixed which allowed to run all XUnit tests in single batch job.

PR Checklist

Remove unneeded dotnet restore Add --no-restore Add extraParams for MacOS Fix TestRunspaceWithPowerShellAndInitialSessionState Fix race condition to access powershell.config.json Reduce an impact of the inter process race condition Fix race condition to access powershell.config.json (PowerShell#8249)

iSazonov · 2018-11-30T07:26:30Z

@daxian-dbw @TravisEz13 @adityapatwardhan Could you please review the PR? It blocks my follow PRs (one could be used to speed up startup).

adityapatwardhan · 2018-11-30T18:38:23Z

test/xUnit/Asserts/PriorityOrderer.cs

@@ -0,0 +1,45 @@
+using System;


Please add copyright header.

adityapatwardhan · 2018-11-30T18:38:34Z

test/xUnit/Asserts/PriorityAttribute.cs

@@ -0,0 +1,12 @@
+using System;


Please add copyright header.

Still missing copyright header here.

Sorry, I lost new commits.

Fixed.

adityapatwardhan · 2018-11-30T18:39:49Z

build.psm1

+        }

        # we are having intermittent issues on macOS with these tests failing.
        # VSTS has suggested forcing them to be sequential


Can we validate if this is still the case. If not, we can removed the condition.

Perhaps @TravisEz13 can comment this.

We would have to try it to know.

We could open a tracking issue for the work and don't block the PR.

Also we have many CodeFactor issues. Also we need to have more tests for binary APIs. Also we need to have performance tests. I think we should open new tracking issues.

adityapatwardhan · 2018-11-30T18:44:56Z

test/xUnit/csharp/test_Runspace.cs

        public void TestRunspaceWithPowerShellAndInitialSessionState()
        {
-            InitialSessionState iss = InitialSessionState.CreateDefault2();
+            InitialSessionState iss = InitialSessionState.CreateDefault();


Why was this changed?

Two tests above use the CreateDefault(). I guess it was typo. If we need tests for CreateDefault2() I believe it should be added in new PR.

I believe CreateDefault2 is intentional as it creates an ISS without any modules loaded. Please change it back.

Reverted with new comment.

@adityapatwardhan that seems like a method name that probably needs revising, if we can, or adding an alias for in some fashion. Ilya is generally pretty familiar with the repo; if he got it confused, it's likely others will also.

Changing the methodName would be a huge breaking change, but we could add a new method that calls this that is named better. I would suggest filing an issue about this.

adityapatwardhan · 2018-12-06T00:55:33Z

@iSazonov Please resolve the merge conflict.

adityapatwardhan · 2018-12-06T00:56:01Z

test/xUnit/xUnit.tests.csproj

Why was this added?

It was temporary additions.

Removed.

iSazonov · 2018-12-06T05:26:51Z

I restarted CI-Windows - there was a race condition again. Perhaps it is only test specific issue. @adityapatwardhan @daxian-dbw could you please look? There test cleanup code failed but I don't understand that process lock the config file. I guess it is other test processes. In this case we need improve the test cleanup code like we did in WaitFile().

iSazonov · 2018-12-07T07:41:54Z

I updated #8400 to fix the rare race condition in tests.

@adityapatwardhan We could merge.

iSazonov · 2018-12-11T18:43:45Z

@adityapatwardhan Could you please update your code review and merge?

TravisEz13 · 2018-12-11T22:42:04Z

@iSazonov @adityapatwardhan Is out of the office until the new year.

iSazonov · 2018-12-12T05:43:55Z

@TravisEz13 If you could merge I could continue my work.

Remove unneeded dotnet restore Add --no-restore Add extraParams for MacOS Fix TestRunspaceWithPowerShellAndInitialSessionState Fix race condition to access powershell.config.json Reduce an impact of the inter process race condition Fix race condition to access powershell.config.json (PowerShell#8249)

TravisEz13 · 2018-12-12T21:12:53Z

rebased the branch mainly to verify the tests still pass

…Shell into move-xunit-tests2

iSazonov · 2018-12-13T12:36:28Z

The test TestRunspaceWithPowerShellAndInitialSessionState fail exclusively and randomly on MacOs if we use CreateDefault2(). The test passes with CreateDefault(). I guess that previously we hided an race condition issue on MacOs by using sequential tests.
I added the issue to #8400 for tracking the issue and added commit to use ``CreateDefault()`.

Aditya is out of the office

iSazonov requested review from PaulHigin, TravisEz13, adityapatwardhan, anmenaga and daxian-dbw as code owners November 29, 2018 05:11

iSazonov mentioned this pull request Nov 29, 2018

Move xUnit tests to new folder #8237

Closed

11 tasks

iSazonov added the CL-Test Indicates that a PR should be marked as a test change in the Change Log label Nov 29, 2018

adityapatwardhan previously requested changes Nov 30, 2018

View reviewed changes

iSazonov added 3 commits December 1, 2018 01:01

Add copyright

664131d

Add copyright 2

d881c3a

Revert to CreateDefault2()

2f5182f

iSazonov force-pushed the move-xunit-tests2 branch from 2f5182f to 59c7e7a Compare December 3, 2018 09:19

adityapatwardhan reviewed Dec 6, 2018

View reviewed changes

iSazonov force-pushed the move-xunit-tests2 branch from 51803cb to 65079e4 Compare December 6, 2018 03:23

TravisEz13 self-assigned this Dec 12, 2018

iSazonov added 4 commits December 12, 2018 13:11

Add copyright

ca3243d

Revert to CreateDefault2

422ebf2

Remove GenerateProgramFile

f6512de

TravisEz13 force-pushed the move-xunit-tests2 branch from 65079e4 to f6512de Compare December 12, 2018 21:12

Merge branch 'move-xunit-tests2' of https://github.com/iSazonov/Power…

d74fa5b

…Shell into move-xunit-tests2

Use CreateDefault() in TestRunspaceWithPowerShellAndInitialSessionState

3e6fc6d

TravisEz13 approved these changes Dec 13, 2018

View reviewed changes

TravisEz13 merged commit a9f106d into PowerShell:master Dec 13, 2018

iSazonov deleted the move-xunit-tests2 branch December 14, 2018 06:24

vexx32 mentioned this pull request Dec 16, 2018

Add 5 'InvokeAsync' overloads and 'StopAsync' to the 'PowerShell' type #8056

Merged

11 tasks

markekraus mentioned this pull request Jan 24, 2019

Update path of test\xUnit\xUnit.tests.csproj in PowerShell.sln #8730

Merged

11 tasks

daxian-dbw mentioned this pull request Feb 22, 2019

Make xUnit test run sequentially to rule out race conditions around 'powershell.config.json' #8945

Merged

11 tasks

Conversation

iSazonov commented Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

PR Summary

PR Checklist

Uh oh!

iSazonov commented Nov 30, 2018

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!

iSazonov Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Dec 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Dec 7, 2018

Uh oh!

iSazonov commented Dec 11, 2018

Uh oh!

TravisEz13 commented Dec 11, 2018

Uh oh!

iSazonov commented Dec 12, 2018

Uh oh!

TravisEz13 commented Dec 12, 2018

Uh oh!

iSazonov commented Dec 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iSazonov commented Nov 29, 2018 •

edited

Loading

iSazonov Nov 30, 2018 •

edited

Loading

iSazonov Dec 1, 2018 •

edited

Loading

iSazonov commented Dec 6, 2018 •

edited

Loading