Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Tests for ConfirmImpact Ratings #8214

Merged
merged 8 commits into from Dec 20, 2018

Conversation

@vexx32
Copy link
Contributor

commented Nov 8, 2018

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

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Nov 9, 2018

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

test/powershell/engine/Basic/DefaultConfirmImpact.Tests.ps1 Outdated Show resolved Hide resolved
test/powershell/engine/Basic/DefaultConfirmImpact.Tests.ps1 Outdated Show resolved Hide resolved

@vexx32 vexx32 changed the title WIP: Add Tests for ConfirmImpact Ratings Add Tests for ConfirmImpact Ratings Nov 9, 2018

test/powershell/engine/Basic/DefaultConfirmImpact.Tests.ps1 Outdated Show resolved Hide resolved

@iSazonov iSazonov requested review from SteveL-MSFT and adityapatwardhan Nov 9, 2018

@iSazonov iSazonov self-assigned this Nov 9, 2018

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Nov 14, 2018

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

test/powershell/engine/Basic/DefaultConfirmImpact.Tests.ps1 Outdated Show resolved Hide resolved
test/powershell/engine/Basic/DefaultConfirmImpact.Tests.ps1 Outdated Show resolved Hide resolved
test/powershell/engine/Basic/DefaultConfirmImpact.Tests.ps1 Outdated Show resolved Hide resolved
test/powershell/engine/Basic/DefaultConfirmImpact.Tests.ps1 Outdated Show resolved Hide resolved
test/powershell/engine/Basic/DefaultConfirmImpact.Tests.ps1 Outdated Show resolved Hide resolved
test/powershell/engine/Basic/DefaultConfirmImpact.Tests.ps1 Outdated Show resolved Hide resolved

@iSazonov iSazonov added the CL-Test label Nov 16, 2018

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Nov 21, 2018

@vexx32 Please address @SteveL-MSFT comments.

@vexx32 vexx32 force-pushed the vexx32:Test-ConfirmImpact branch to 2804c0e Nov 21, 2018

@vexx32

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Collaborator

commented Nov 21, 2018

$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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2018

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

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Nov 29, 2018

@vexx32 Please resolve the merge conflict.

@SteveL-MSFT Please update your code review.

@vexx32 vexx32 force-pushed the vexx32:Test-ConfirmImpact branch from 9020fa9 to b93abcb Nov 29, 2018

@vexx32

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2018

There we go, all done! 😄

@adityapatwardhan

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2018

@adityapatwardhan Tests and PR description updated!

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Dec 6, 2018

@vexx32 Please resolve the merge conflit.

@SteveL-MSFT Please update your code review.

5 similar comments
@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Dec 6, 2018

@vexx32 Please resolve the merge conflit.

@SteveL-MSFT Please update your code review.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Dec 6, 2018

@vexx32 Please resolve the merge conflit.

@SteveL-MSFT Please update your code review.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Dec 6, 2018

@vexx32 Please resolve the merge conflit.

@SteveL-MSFT Please update your code review.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Dec 6, 2018

@vexx32 Please resolve the merge conflit.

@SteveL-MSFT Please update your code review.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Dec 6, 2018

@vexx32 Please resolve the merge conflit.

@SteveL-MSFT Please update your code review.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Dec 6, 2018

@vexx32 Please resolve the merge conflit.

@SteveL-MSFT Please update your code review.

vexx32 added some commits Nov 8, 2018

Add file to test ConfirmImpact ratings of commands
Also test that number of cmdlets matches what is available.
Use more direct method of determining the commands available
Might be needed since unix doesn't have some commands

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

Remove unnecessary sort
Attempt to fix weird issues
Use proper import of default modules

Remove unneeded check

Fix spacing
Update defaultCommands tests to include ConfirmImpact
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 vexx32 force-pushed the vexx32:Test-ConfirmImpact branch from 149668d to e909162 Dec 6, 2018

@vexx32

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Collaborator

commented Dec 7, 2018

@vexx32 Please resolve the merge conflit.

@SteveL-MSFT Please update your code review.

@vexx32

This comment has been minimized.

Copy link
Contributor Author

commented Dec 16, 2018

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

@SteveL-MSFT
Copy link
Member

left a comment

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

@vexx32

This comment has been minimized.

Copy link
Contributor Author

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

7 checks passed

CodeFactor No issues found.
Details
PowerShell-CI-linux #PR-8214-20181206.01 succeeded
Details
PowerShell-CI-macos #PR-8214-20181206.01 succeeded
Details
PowerShell-CI-spelling #PR-8214-20181206.01 succeeded
Details
PowerShell-CI-windows #PR-8214-20181206.01 succeeded
Details
WIP Ready for review
Details
license/cla All CLA requirements met.
Details

@vexx32 vexx32 deleted the vexx32:Test-ConfirmImpact branch Dec 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.