GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add Tests for ConfirmImpact Ratings by vexx32 · Pull Request #8214 · PowerShell/PowerShell

Conversation

@vexx32

@vexx32 Rain Sallow (/u/ta11ow) (vexx32) commented Nov 8, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

PR Summary

/cc Ilya (@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

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Change is not breaking
  • Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.
  • User-facing changes
    • Not Applicable
    • OR
    • User-facing Documentation needed
      • Issue filed - Issue link:
  • Testing - New and feature
    • Not Applicable or can only be tested interactively
    • OR
    • Make sure you've added a new test if existing tests do not effectively test the code changed
      • Add [feature] if the change is significant or affects feature tests

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vexx32 Rain Sallow (/u/ta11ow) (vexx32) mentioned this pull request Nov 8, 2018
ConfirmImpact: Correctly Report impact level when SupportsShouldProcess = false #8209
Merged
7 of 11 tasks
@iSazonov

Ilya (iSazonov) commented Nov 9, 2018

Copy link
Copy Markdown
Collaborator

Rain Sallow (/u/ta11ow) (@vexx32) Please look DefaultCommands.Tests.ps1. There is a trick with moderated loading standard modules.
vexx32 reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iSazonov
Ilya (iSazonov) reviewed Nov 9, 2018
View reviewed changes
Comment thread test/powershell/engine/Basic/DefaultConfirmImpact.Tests.ps1 Resolved

Uh oh!

There was an error while loading. Please reload this page.
iSazonov
Ilya (iSazonov) reviewed Nov 9, 2018
View reviewed changes
Comment thread test/powershell/engine/Basic/DefaultConfirmImpact.Tests.ps1 Resolved

Uh oh!

There was an error while loading. Please reload this page.
@vexx32 Rain Sallow (/u/ta11ow) (vexx32) changed the title WIP: Add Tests for ConfirmImpact Ratings Add Tests for ConfirmImpact Ratings Nov 9, 2018
iSazonov
Ilya (iSazonov) reviewed Nov 9, 2018
View reviewed changes
Comment thread test/powershell/engine/Basic/DefaultConfirmImpact.Tests.ps1 Resolved

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov
Ilya (iSazonov) requested review from Steve Lee (SteveL-MSFT) and Aditya Patwardhan (adityapatwardhan) November 9, 2018 04:31
@iSazonov Ilya (iSazonov) self-assigned this Nov 9, 2018
iSazonov
Ilya (iSazonov) approved these changes Nov 9, 2018
View reviewed changes
@iSazonov

Ilya (iSazonov) commented Nov 14, 2018

Copy link
Copy Markdown
Collaborator

Steve Lee (@SteveL-MSFT) Aditya Patwardhan (@adityapatwardhan) Could you please review these new tests?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
SteveL-MSFT
Steve Lee (SteveL-MSFT) requested changes Nov 14, 2018
View reviewed changes
Comment thread test/powershell/engine/Basic/DefaultConfirmImpact.Tests.ps1 Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread test/powershell/engine/Basic/DefaultConfirmImpact.Tests.ps1 Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread test/powershell/engine/Basic/DefaultConfirmImpact.Tests.ps1 Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread test/powershell/engine/Basic/DefaultConfirmImpact.Tests.ps1 Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread test/powershell/engine/Basic/DefaultConfirmImpact.Tests.ps1 Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread test/powershell/engine/Basic/DefaultConfirmImpact.Tests.ps1 Resolved

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov Ilya (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

Ilya (iSazonov) commented Nov 21, 2018

Copy link
Copy Markdown
Collaborator

Rain Sallow (/u/ta11ow) (@vexx32) Please address Steve Lee (@SteveL-MSFT) comments.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vexx32
Rain Sallow (/u/ta11ow) (vexx32) force-pushed the Test-ConfirmImpact branch from 49b5e32 to 2804c0e Compare November 21, 2018 04:55
@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Nov 21, 2018

Copy link
Copy Markdown
Collaborator Author

Ilya (@iSazonov) built ConfirmImpact tests into the existing file per Steve Lee (@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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Nov 21, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

$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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Nov 22, 2018

Copy link
Copy Markdown
Collaborator Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Nov 29, 2018

Copy link
Copy Markdown
Collaborator

Rain Sallow (/u/ta11ow) (@vexx32) Please resolve the merge conflict.

Steve Lee (@SteveL-MSFT) Please update your code review.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vexx32
Rain Sallow (/u/ta11ow) (vexx32) force-pushed the Test-ConfirmImpact branch from 9020fa9 to b93abcb Compare November 29, 2018 15:21
@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Nov 29, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

There we go, all done! 😄
iSazonov reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
adityapatwardhan
Aditya Patwardhan (adityapatwardhan) approved these changes Nov 29, 2018
View reviewed changes
@adityapatwardhan

Aditya Patwardhan (adityapatwardhan) commented Nov 29, 2018

Copy link
Copy Markdown
Member

Rain Sallow (/u/ta11ow) (@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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Nov 29, 2018

Copy link
Copy Markdown
Collaborator Author

Aditya Patwardhan (@adityapatwardhan) Tests and PR description updated!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Dec 6, 2018

Copy link
Copy Markdown
Collaborator

Rain Sallow (/u/ta11ow) (@vexx32) Please resolve the merge conflit.

Steve Lee (@SteveL-MSFT) Please update your code review.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
5 similar comments
@iSazonov

Ilya (iSazonov) commented Dec 6, 2018

Copy link
Copy Markdown
Collaborator

Rain Sallow (/u/ta11ow) (@vexx32) Please resolve the merge conflit.

Steve Lee (@SteveL-MSFT) Please update your code review.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Dec 6, 2018

Copy link
Copy Markdown
Collaborator

Rain Sallow (/u/ta11ow) (@vexx32) Please resolve the merge conflit.

Steve Lee (@SteveL-MSFT) Please update your code review.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Dec 6, 2018

Copy link
Copy Markdown
Collaborator

Rain Sallow (/u/ta11ow) (@vexx32) Please resolve the merge conflit.

Steve Lee (@SteveL-MSFT) Please update your code review.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Dec 6, 2018

Copy link
Copy Markdown
Collaborator

Rain Sallow (/u/ta11ow) (@vexx32) Please resolve the merge conflit.

Steve Lee (@SteveL-MSFT) Please update your code review.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Dec 6, 2018

Copy link
Copy Markdown
Collaborator

Rain Sallow (/u/ta11ow) (@vexx32) Please resolve the merge conflit.

Steve Lee (@SteveL-MSFT) Please update your code review.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Dec 6, 2018

Copy link
Copy Markdown
Collaborator

Rain Sallow (/u/ta11ow) (@vexx32) Please resolve the merge conflit.

Steve Lee (@SteveL-MSFT) Please update your code review.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Rain Sallow (/u/ta11ow) (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
Rain Sallow (/u/ta11ow) (vexx32) force-pushed the Test-ConfirmImpact branch from 149668d to e909162 Compare December 6, 2018 17:47
@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Dec 6, 2018

Copy link
Copy Markdown
Collaborator Author

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. 😄
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Dec 7, 2018

Copy link
Copy Markdown
Collaborator

Rain Sallow (/u/ta11ow) (@vexx32) Please resolve the merge conflit.

Steve Lee (@SteveL-MSFT) Please update your code review.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Dec 16, 2018

Copy link
Copy Markdown
Collaborator Author

Merge conflict is sorted /cc Steve Lee (@SteveL-MSFT) Ilya (@iSazonov) 😄
iSazonov reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
SteveL-MSFT
Steve Lee (SteveL-MSFT) approved these changes Dec 20, 2018
View reviewed changes

@SteveL-MSFT Steve Lee (SteveL-MSFT) left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Dec 20, 2018

Copy link
Copy Markdown
Collaborator Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov
Ilya (iSazonov) merged commit 64bbdbe into PowerShell:master Dec 20, 2018
@vexx32
Rain Sallow (/u/ta11ow) (vexx32) deleted the Test-ConfirmImpact branch December 20, 2018 18:17
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@vexx32 @iSazonov
Add Tests for ConfirmImpact Ratings (PowerShell#8214)
abaea61
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov Ilya (iSazonov) iSazonov approved these changes
@SteveL-MSFT Steve Lee (SteveL-MSFT) SteveL-MSFT approved these changes
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) adityapatwardhan approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@iSazonov Ilya (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.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@vexx32 @iSazonov @adityapatwardhan @SteveL-MSFT
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.