Remoting and Job tests by adityapatwardhan · Pull Request #4928 · PowerShell/PowerShell

adityapatwardhan · 2017-09-27T01:03:26Z

This should increase coverage under src/System.Management.Automation/engine/remoting substantially.

msftclas · 2017-09-27T01:03:35Z

All CLA requirements met.

PaulHigin · 2017-09-27T16:40:59Z

Why was this check removed?

ComputerName cannot be null due to ParameterValidation. So this.ComputerName == null is always false. Which makes the if always false due to the &&. Hence unreachable code.

Makes sense. Can you also remove the error message from RemotingErrorIdStrings.resx and RemotingErrorIdStrings.cs?

PaulHigin · 2017-09-27T17:00:28Z

I am worried about race condition here. It is possible for the job to complete before the ValidateJobInfo script runs and cause false failures. I dislike adding 'sleep 1' to the job script because it slows down the test.
I think the best thing is to remove this test for the intermediate 'Running' state and just do the following test for 'Completed' state and check the result.

PaulHigin · 2017-09-27T17:37:17Z

I am not as concerned about this intermediate state race condition because the second Start-Job takes time. So we are probably Ok.

PaulHigin · 2017-09-27T17:40:56Z

Minor comment. Should we use an AfterEach { Get-Job | Remove-Job -Force -EA Silently } block to clean up after each It block?

PaulHigin · 2017-09-27T17:57:17Z

This assumes the first job waiting one second will be the first to complete. This will be true most of the time but it is possible for a different job to complete first. I feel the test can allow '1', '2', '3' as the result and still be a good test

PaulHigin · 2017-09-27T21:17:32Z

Was this added to ensure there is a PowerShell 6.0 endpoint available? Or just to ensure remoting is enabled? If so can you add a comment?

PaulHigin · 2017-09-27T21:40:03Z

Typo ... "so that when"...

PaulHigin · 2017-09-27T21:40:59Z

PaulHigin · 2017-09-27T21:47:42Z

I think this should be a "Feature" test. Or at least have the "Slow" tag.

It takes about 10 seconds. I believe it is fast enough to be in CI. Thoughts?

I am not sure but the testing guidelines say most CI tests take less than one second and the "Slow" tag should be considered if a test takes longer. Since these are critical remoting functions it seems like we should leave them as CI tests, but probably should add the "Slow" tag.

PaulHigin · 2017-09-27T21:48:41Z

"Feature" or "Slow" tag?

These finish in about 12 seconds. I think that should be OK. Thoughts?

I am not sure but the testing guidelines say most CI tests take less than one second and the "Slow" tag should be considered if a test takes longer. Since these are critical remoting functions it seems like we should leave them as CI tests, but probably should add the "Slow" tag.

PaulHigin

This looks good to me. The only additional feedback I have is:
a. Please also remove InvokeDisconnectWithoutComputerName resource string in InvokeCommandCommand.cs.
b. Consider using 'Slow' tag on the slower remoting tests.

mirichmo · 2017-09-28T01:12:20Z

There seems to be some repetition here in each test case within this context. How about changing the BeforeEach to an AfterEach and removing lines 40, 48, etc...

mirichmo · 2017-09-28T01:18:11Z

AfterEach cleanup is needed within this Context as well.

Comment applies to all Context blocks.

mirichmo · 2017-09-28T01:19:17Z

Organizational nitpick: This is a Receive-Job test that is included within the Wait-Job set of tests

Moved. Good catch.

mirichmo · 2017-09-28T01:24:25Z

Would a check to make sure that none of the jobs threw errors make sense here too?

mirichmo · 2017-09-28T23:37:10Z

The scriptblock { 1 + 1 } and string ' 1 + 1 ' are repeated throughout This is one of those readability vs changeability trade offs. Did you consider making them script variables?

I preferred readability. Likelihood of we needing to change is low. Your thoughts?

mirichmo · 2017-09-29T16:24:43Z

I've had a review going for a few days. I published the comments so that I can sync it based on your latest updates

mirichmo · 2017-09-29T16:30:28Z

This test doesn't actually test the NoRecurse functionality. There should be a child job and a check should be made to ensure that it gets the correct value.

All jobs create a childjob which returns the result. Hence with NoRecurse we cannot get the result.

mirichmo · 2017-09-29T16:34:23Z

I prefer AfterEach for clean up. With BeforeEach, the last It will "leak" a Job that it should clean up.

mirichmo · 2017-09-29T16:35:15Z

Nit: Please capitalize Get-Job

mirichmo · 2017-09-29T16:37:27Z

Is there an issue for this in the Pester repo? If so, please include a link

I need to do a deeper analysis to file an issue. It could be Pester or PowerShell scoping issue.

mirichmo · 2017-09-29T16:38:17Z

Clean up should be done in an AfterEach. Remove-Job won't execute if the ValidateJobInfo test fails.

mirichmo · 2017-09-29T16:44:10Z

This session won't get cleaned up during test failure. Comment also applies to the other It's within this Context.

Fixed. Used try/finally

mirichmo · 2017-09-29T16:49:00Z

This test doesn't validate that the scriptblock was run in the correct scope. It just validates that the scriptblock was run in a scope. It provides code coverage, but isn't really useful in terms of validating functionality as-is.

Fixed. I set a variable in the script block and check the value after.

mirichmo · 2017-09-29T16:51:44Z

remove-job?

Added -AutoRemove.

adityapatwardhan · 2017-09-29T19:32:07Z

@mirichmo Resolved all your comments. Please have another look.

mirichmo · 2017-09-29T20:33:05Z

mirichmo · 2017-09-29T20:34:23Z

Can you move this comment to line 271? It makes more sense there

adityapatwardhan · 2017-09-30T01:37:16Z

@mirichmo Resolved all feedback. Please have a look. Unrelated tests failing.

TravisEz13 · 2017-10-02T17:29:45Z

@mirichmo Can you update your review?

adityapatwardhan requested review from JamesWTruher and PaulHigin September 27, 2017 01:03

adityapatwardhan requested review from dantraMSFT and mirichmo as code owners September 27, 2017 01:03

adityapatwardhan closed this Sep 27, 2017

adityapatwardhan reopened this Sep 27, 2017

adityapatwardhan force-pushed the RemotingTests branch from 7b504b4 to 194b9e7 Compare September 27, 2017 19:13

daxian-dbw assigned TravisEz13 Sep 27, 2017

PaulHigin suggested changes Sep 27, 2017

View reviewed changes

adityapatwardhan requested review from TravisEz13 and daxian-dbw as code owners September 28, 2017 00:38

PaulHigin approved these changes Sep 28, 2017

View reviewed changes

mirichmo suggested changes Sep 29, 2017

View reviewed changes

adityapatwardhan force-pushed the RemotingTests branch from aa69842 to 2444dc7 Compare September 29, 2017 19:31

mirichmo reviewed Sep 29, 2017

View reviewed changes

adityapatwardhan force-pushed the RemotingTests branch from b9138f4 to ab1f645 Compare September 30, 2017 00:08

adityapatwardhan added 8 commits October 2, 2017 09:45

Remoting and Job tests

b77439f

[Feature] fix test issues

04e1e89

Skipping remoting tests on Non-Windows

fc61c0f

Use HelpersRemoting for creating PSSession

0f83e79

[Feature] Fix failures in CI

dd9e137

[Feature] Fix typo

e978771

Test fixes and address CR comments

ad62f14

[Feature] Test fixes

fb69dff

adityapatwardhan force-pushed the RemotingTests branch from ab1f645 to fb69dff Compare October 2, 2017 16:45

mirichmo approved these changes Oct 2, 2017

View reviewed changes

TravisEz13 merged commit 1e271ea into PowerShell:master Oct 2, 2017

mirichmo mentioned this pull request Oct 4, 2017

Test failures in *PSSessionConfiguration tests #4535

Closed

adityapatwardhan deleted the RemotingTests branch November 1, 2018 17:38

Conversation

adityapatwardhan commented Sep 27, 2017

Uh oh!

msftclas commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

PaulHigin left a comment

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!

mirichmo commented Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

msftclas commented Sep 27, 2017 •

edited

Loading

mirichmo commented Sep 29, 2017 •

edited

Loading