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 — Tests for New-WinEvent by anmenaga · Pull Request #4384 · PowerShell/PowerShell

Conversation

@anmenaga

@anmenaga Andrew (anmenaga) commented Jul 28, 2017

Copy link
Copy Markdown

Some tests for New-WinEvent that bring code coverage of the class to 80%.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Andrew
New-WinEvent tests
72cca70
@anmenaga
Andrew (anmenaga) requested a review from Steve Lee (SteveL-MSFT) July 28, 2017 22:24
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Jul 28, 2017
adityapatwardhan
Aditya Patwardhan (adityapatwardhan) requested changes Jul 28, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Diagnostics/New-WinEvent.Tests.ps1 Outdated

Context "New-WinEvent tests" {

It 'Simple New-WinEvent' -Skip:(-not $IsWindows) {

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Jul 28, 2017

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.

You may want to use $PSDefaultParameterValues to set the value of skip.

Sorry, something went wrong.

Uh oh!

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

@anmenaga Andrew (anmenaga) Jul 31, 2017

Copy link
Copy Markdown
Author

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.

Yup, I saw that approach; but don't really like it; I think having skip conditions explicitly on each test makes it easier to investigate test failures.

Sorry, something went wrong.

Uh oh!

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

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Jul 31, 2017

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.

The recommendation according to the testing guidelines is to use PSDefaultParameterValues. Please see: https://github.com/PowerShell/PowerShell/blob/master/docs/testing-guidelines/WritingPesterTests.md

Sorry, something went wrong.

Uh oh!

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

@anmenaga Andrew (anmenaga) Jul 31, 2017

Copy link
Copy Markdown
Author

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.

Updated.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Diagnostics/New-WinEvent.Tests.ps1 Outdated
It 'Simple New-WinEvent' -Skip:(-not $IsWindows) {
New-WinEvent -ProviderName Microsoft-Windows-PowerShell -Id 40962 -Version 1 # simple event without any payload
$filter = @{ ProviderName = 'Microsoft-Windows-PowerShell'; Id = 40962}
(Get-WinEvent -filterHashtable $filter).Count -gt 0 | Should be $true

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Jul 28, 2017

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.

Should be,

(Get-WinEvent -filterHashtable $filter).Count | Should BeGreaterThan 0

Sorry, something went wrong.

Uh oh!

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

@anmenaga Andrew (anmenaga) Jul 31, 2017

Copy link
Copy Markdown
Author

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.

Updated.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
iSazonov
Ilya (iSazonov) reviewed Jul 31, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Diagnostics/New-WinEvent.Tests.ps1 Outdated
}

It 'No provider found error' -Skip:(-not $IsWindows) {
{ New-WinEvent -ProviderName NonExistingProvider -Id 0 } | ShouldBeErrorID 'System.ArgumentException,Microsoft.PowerShell.Commands.NewWinEventCommand'

@iSazonov Ilya (iSazonov) Jul 31, 2017

Copy link
Copy Markdown
Collaborator

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.

Typo. Please change "ShouldBeErrorID" -> "ShouldBeErrorId".

Sorry, something went wrong.

Uh oh!

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

@anmenaga Andrew (anmenaga) Jul 31, 2017

Copy link
Copy Markdown
Author

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.

Updated.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Diagnostics/New-WinEvent.Tests.ps1 Outdated

It 'PayloadMismatch error' -Skip:(-not $IsWindows) {
$logPath = join-path $TestDrive 'testlog1.txt'
New-WinEvent -ProviderName Microsoft-Windows-PowerShell -Id 32868 *> $logPath # this will print the warning with expected event template to the file

@iSazonov Ilya (iSazonov) Jul 31, 2017

Copy link
Copy Markdown
Collaborator

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.

Please move the comment on separate line.

Sorry, something went wrong.

Uh oh!

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

@anmenaga Andrew (anmenaga) Jul 31, 2017

Copy link
Copy Markdown
Author

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.

Moved.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@daxian-dbw Dongbo Wang (daxian-dbw) assigned Aditya Patwardhan (adityapatwardhan) Jul 31, 2017
Andrew added 2 commits July 31, 2017 16:19
Andrew
PR feedback
6411500
Andrew
PR feedback 2
7da758f
adityapatwardhan
Aditya Patwardhan (adityapatwardhan) requested changes Aug 1, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Diagnostics/New-WinEvent.Tests.ps1 Outdated
}

It 'Simple New-WinEvent' {
New-WinEvent -ProviderName Microsoft-Windows-PowerShell -Id 40962 -Version 1 # simple event without any payload

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Aug 1, 2017

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.

Move the comment to a separate line.

Sorry, something went wrong.

Uh oh!

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

@anmenaga Andrew (anmenaga) Aug 1, 2017

Copy link
Copy Markdown
Author

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.

updated.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
SteveL-MSFT
Steve Lee (SteveL-MSFT) requested changes Aug 1, 2017
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.

Which issue does this address if any?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Diagnostics/New-WinEvent.Tests.ps1 Outdated
}

It 'Simple New-WinEvent' {
New-WinEvent -ProviderName Microsoft-Windows-PowerShell -Id 40962 -Version 1 # simple event without any payload

@SteveL-MSFT Steve Lee (SteveL-MSFT) Aug 1, 2017

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.

use something like

$PSProviderId = 40962

Sorry, something went wrong.

Uh oh!

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

@anmenaga Andrew (anmenaga) Aug 1, 2017

Copy link
Copy Markdown
Author

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.

Updated.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Andrew
PR feedback 3
fe2df53
@anmenaga

Andrew (anmenaga) commented Aug 1, 2017

Copy link
Copy Markdown
Author

This goes toward #4156
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Andrew
PR feedback 4
b3d408c
adityapatwardhan
Aditya Patwardhan (adityapatwardhan) approved these changes Aug 1, 2017
View reviewed changes
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Aug 2, 2017

Copy link
Copy Markdown
Member

Based on the code coverage, you're missing some tests:
https://codecov.io/gh/PowerShell/PowerShell/src/9e41c647a9407e05d86f4971a6bd25719a7130d0/src/Microsoft.PowerShell.CoreCLR.Eventing/DotNetCode/Eventing/EventDescriptor.cs

  • EventDescriptor constructor validation like: [System.Diagnostics.Eventing.EventDescriptor]::new(-1, 0, 0, 0, 0, -1, 0)
  • Reading back members of EventDescriptor
    https://codecov.io/gh/PowerShell/PowerShell/src/9e41c647a9407e05d86f4971a6bd25719a7130d0/src/Microsoft.PowerShell.CoreCLR.Eventing/DotNetCode/Eventing/EventProvider.cs
  • Dispose(), Close(), destructor
  • IsEnabled()
  • different data types
  • WriteMessagEvent()
  • WriteEvent
  • WriteTransferEvent
    https://codecov.io/gh/PowerShell/PowerShell/src/9e41c647a9407e05d86f4971a6bd25719a7130d0/src/Microsoft.PowerShell.CoreCLR.Eventing/DotNetCode/Eventing/EventProviderTraceListener.cs
  • EventProviderTraceListener

The Reader code should probably be a different PR
All reactions

Sorry, something went wrong.

Uh oh!

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

Andrew (anmenaga) commented Aug 2, 2017

Copy link
Copy Markdown
Author

Steve Lee (@SteveL-MSFT) All of the things that you've mentioned is ETW code and will go into separate PR.
This PR is specific to New-WinEvent cmdlet code.
All reactions

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 Aug 2, 2017
View reviewed changes
@adityapatwardhan

Aditya Patwardhan (adityapatwardhan) commented Aug 3, 2017

Copy link
Copy Markdown
Member

Ilya (@iSazonov) Can you have a look again?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iSazonov
Ilya (iSazonov) reviewed Aug 4, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Diagnostics/New-WinEvent.Tests.ps1 Outdated
}

It 'Simple New-WinEvent' {
# simple event without any payload

@iSazonov Ilya (iSazonov) Aug 4, 2017

Copy link
Copy Markdown
Collaborator

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.

I believe we can remove the comment. And maybe move to the test name.

Sorry, something went wrong.

Uh oh!

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

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Aug 4, 2017

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.

Andrew (@anmenaga) can you address this?

Sorry, something went wrong.

Uh oh!

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

@anmenaga Andrew (anmenaga) Aug 4, 2017

Copy link
Copy Markdown
Author

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.

Updated.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iSazonov reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@iSazonov

Ilya (iSazonov) commented Aug 4, 2017

Copy link
Copy Markdown
Collaborator

With one minor comment LGTM.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Andrew
PR feedback 5
6d8d616
adityapatwardhan
Aditya Patwardhan (adityapatwardhan) approved these changes Aug 4, 2017
View reviewed changes
@adityapatwardhan
Aditya Patwardhan (adityapatwardhan) merged commit 0b7451b into PowerShell:master Aug 7, 2017
@anmenaga
Andrew (anmenaga) deleted the NewWinEventTests branch October 31, 2018 21:19
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
Andrew @adityapatwardhan
Tests for New-WinEvent (PowerShell#4384)
ee905c6 
* New-WinEvent tests

* PR feedback

* PR feedback 2

* PR feedback 3

* PR feedback 4

* PR feedback 5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov Ilya (iSazonov) iSazonov left review comments
@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

@adityapatwardhan Aditya Patwardhan (adityapatwardhan)

Labels

None yet

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.

5 participants

@anmenaga @SteveL-MSFT @adityapatwardhan @iSazonov @msftclas
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.