Skip to content

Add Tests for ConfirmImpact Ratings#8214

Merged
iSazonov merged 8 commits intoPowerShell:masterfrom
vexx32:Test-ConfirmImpact
Dec 20, 2018
Merged

Add Tests for ConfirmImpact Ratings#8214
iSazonov merged 8 commits intoPowerShell:masterfrom
vexx32:Test-ConfirmImpact

Conversation

@vexx32
Copy link
Collaborator

@vexx32 vexx32 commented Nov 8, 2018 •
edited
Loading

PR Summary

/cc @iSazonov

Adds aspect to tests in test/powershell/engine/Basic/DefaultCommands.Tests.ps1 that checks default cmdlets' reported ConfirmImpact level.

Also fixes a related logical error in /tests/powershell/Language/Classes/Scripting.Classes.BasicParsing.Tests.ps1 where the test attempts to query the wrong property on the class object, and then tests for ConfirmImpact rating without implementing ShouldProcess.

Related: #8209
These tests will be modified in #8209 when needed.

PR Checklist

@vexx32 vexx32 mentioned this pull request Nov 8, 2018
Merged
11 tasks
@iSazonov
Copy link
Collaborator

iSazonov commented Nov 9, 2018

@vexx32 Please look DefaultCommands.Tests.ps1. There is a trick with moderated loading standard modules.

iSazonov
iSazonov reviewed Nov 9, 2018
View reviewed changes
iSazonov
iSazonov reviewed Nov 9, 2018
View reviewed changes
@vexx32 vexx32 changed the title WIP: Add Tests for ConfirmImpact Ratings Add Tests for ConfirmImpact Ratings Nov 9, 2018
iSazonov
iSazonov reviewed Nov 9, 2018
View reviewed changes
@iSazonov iSazonov self-assigned this Nov 9, 2018
@iSazonov
Copy link
Collaborator

iSazonov commented Nov 14, 2018

@SteveL-MSFT @adityapatwardhan Could you please review these new tests?

SteveL-MSFT
SteveL-MSFT requested changes Nov 14, 2018
View reviewed changes
@iSazonov iSazonov added the CL-Test Indicates that a PR should be marked as a test change in the Change Log label Nov 16, 2018
@iSazonov
Copy link
Collaborator

iSazonov commented Nov 21, 2018

@vexx32 Please address @SteveL-MSFT comments.

@vexx32 vexx32 force-pushed the Test-ConfirmImpact branch from 49b5e32 to 2804c0e Compare November 21, 2018 04:55
@vexx32
Copy link
Collaborator Author

vexx32 commented Nov 21, 2018

@iSazonov built ConfirmImpact tests into the existing file per @SteveL-MSFT's recommendations. Haven't tested it just yet, but it should be sound. I'll let CI check it while I get some sleep and figure out any issues tomorrow.

Also did a rebase due to some outstanding merge conflicts & resolved those issues.

@SteveL-MSFT I notice that there are references to $FullCLR in this file; are these vestiges from Windows PowerShell that can be removed unilaterally?

@iSazonov
Copy link
Collaborator

iSazonov commented Nov 21, 2018 •
edited
Loading

$FullCLR helps to see the differences from Windows PowerShell, helps to document. Also we can run the test with Windows PowerShell to keep it up-to-date. From the test we can see that we still need to port.

@vexx32
Copy link
Collaborator Author

vexx32 commented Nov 22, 2018

Okay, it took me a bit to get that working as it perhaps should, but there we go. 😄

@iSazonov
Copy link
Collaborator

iSazonov commented Nov 29, 2018

@vexx32 Please resolve the merge conflict.

@SteveL-MSFT Please update your code review.

@vexx32 vexx32 force-pushed the Test-ConfirmImpact branch from 9020fa9 to b93abcb Compare November 29, 2018 15:21
@vexx32
Copy link
Collaborator Author

vexx32 commented Nov 29, 2018 •
edited
Loading

There we go, all done! 😄

@adityapatwardhan
Copy link
Member

adityapatwardhan commented Nov 29, 2018

@vexx32 Please have a look at the test failures on macOS and Linux CI.

Also, please update the PR description to remove reference to : DefaultConfirmImpact.Tests.ps1

@vexx32
Copy link
Collaborator Author

vexx32 commented Nov 29, 2018

@adityapatwardhan Tests and PR description updated!

@iSazonov
Copy link
Collaborator

iSazonov commented Dec 6, 2018

@vexx32 Please resolve the merge conflit.

@SteveL-MSFT Please update your code review.

5 similar comments
@iSazonov
Copy link
Collaborator

iSazonov commented Dec 6, 2018

@vexx32 Please resolve the merge conflit.

@SteveL-MSFT Please update your code review.

@iSazonov
Copy link
Collaborator

iSazonov commented Dec 6, 2018

@vexx32 Please resolve the merge conflit.

@SteveL-MSFT Please update your code review.

@iSazonov
Copy link
Collaborator

iSazonov commented Dec 6, 2018

@vexx32 Please resolve the merge conflit.

@SteveL-MSFT Please update your code review.

@iSazonov
Copy link
Collaborator

iSazonov commented Dec 6, 2018

@vexx32 Please resolve the merge conflit.

@SteveL-MSFT Please update your code review.

@iSazonov
Copy link
Collaborator

iSazonov commented Dec 6, 2018

@vexx32 Please resolve the merge conflit.

@SteveL-MSFT Please update your code review.

@iSazonov
Copy link
Collaborator

iSazonov commented Dec 6, 2018

@vexx32 Please resolve the merge conflit.

@SteveL-MSFT Please update your code review.

vexx32 added 8 commits December 6, 2018 12:21
@vexx32
Fix broken test to properly apply attribute properties it was expecting
6f47bd7
@vexx32
Add file to test ConfirmImpact ratings of commands
adc6b8a 
Also test that number of cmdlets matches what is available.
@vexx32
Use more direct method of determining the commands available
3266a22 
Might be needed since unix doesn't have some commands

soft-fail for missing cmdlets on unix OSes for ConfirmImpact checks
@vexx32
Add more robust test for command availability
7a6c091 
Fix typos

Remove unnecessary sort
@vexx32
Attempt to fix weird issues
313978e 
Use proper import of default modules

Remove unneeded check

Fix spacing
@vexx32
Update defaultCommands tests to include ConfirmImpact
ca5c8cc 
Remove unneeded additional test file

Update tests with -Because for easier debugging

Remove tabs for test CSV string

Reorganise test

I think I fixed it
(famous last words, eh?)

Add back missing Present flags for PSHostProcess items
@vexx32
Let test show cmdlet name for failing confirmimpact items
48db986
@vexx32
Add $CoreUnix specifier to New-PSSessionOption
e909162
@vexx32 vexx32 force-pushed the Test-ConfirmImpact branch from 149668d to e909162 Compare December 6, 2018 17:47
@vexx32
Copy link
Collaborator Author

vexx32 commented Dec 6, 2018

I think that takes care of it. Managed to figure out which commands have been added since last commit, so hopefully that should be sufficient, but I'm gonna wait and see if CI reckons I missed something and fix it as it happens. 😄

@iSazonov
Copy link
Collaborator

iSazonov commented Dec 7, 2018

@vexx32 Please resolve the merge conflit.

@SteveL-MSFT Please update your code review.

@vexx32
Copy link
Collaborator Author

vexx32 commented Dec 16, 2018

Merge conflict is sorted /cc @SteveL-MSFT @iSazonov 😄

SteveL-MSFT
SteveL-MSFT approved these changes Dec 20, 2018
View reviewed changes
Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, been busy and then got backlogged on 90 GitHub notifications but finally got to this one

@vexx32
Copy link
Collaborator Author

vexx32 commented Dec 20, 2018

All good, I imagine it gets fun around the holidays over there. 😄

@iSazonov iSazonov merged commit 64bbdbe into PowerShell:master Dec 20, 2018
@vexx32 vexx32 deleted the Test-ConfirmImpact branch December 20, 2018 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov approved these changes

@SteveL-MSFT SteveL-MSFT SteveL-MSFT approved these changes

@adityapatwardhan adityapatwardhan adityapatwardhan approved these changes

Assignees

@iSazonov iSazonov

Labels

CL-Test Indicates that a PR should be marked as a test change in the Change Log

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vexx32 @iSazonov @adityapatwardhan @SteveL-MSFT

Comments