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

Added the ability to change the power options on the network adapter #219

Closed
wants to merge 7 commits into from

Conversation

edycus
Copy link

@edycus edycus commented Jun 16, 2017

This change is Reviewable

@codecov-io
Copy link

codecov-io commented Jun 16, 2017

Codecov Report

Merging #219 into dev will decrease coverage by 1%.
The diff coverage is 2%.

Impacted file tree graph

@@         Coverage Diff         @@
##            dev   #219   +/-   ##
===================================
- Coverage    96%    94%   -2%     
===================================
  Files        17     18    +1     
  Lines      1276   1358   +82     
===================================
+ Hits       1228   1285   +57     
- Misses       48     73   +25

@edycus
Copy link
Author

edycus commented Jun 16, 2017

Now what?

@PlagueHO
Copy link
Member

Hi @edycus - there are a few issues that are causing the tests to fail - nothing major though:

  1. File MSFT_xNetPowerManagement.psm1 needs to be converted to UTF-8 (https://ci.appveyor.com/project/PowerShell/xnetworking/build/3.1.567.0#L2405). An easy way of doing this is just to open the file in VS Code and use the "Change File encoding" command:
    image

  2. File MSFT_xNetPowerManagement.schema.mof needs to be converted to ASCII (https://ci.appveyor.com/project/PowerShell/xnetworking/build/3.1.567.0#L2406) - same solution as above, except different encoding.

  3. Files MSFT_xNetPowerManagement.psm1 and MSFT_xNetPowerManagement.schema.mof should use spaces instead of tabs. I use a VS Code extension called Untabify to helpfully convert all tabs to spaces in my files. Suggest trying that. https://ci.appveyor.com/project/PowerShell/xnetworking/build/3.1.567.0#L2412

  4. There are some PSSA rule violations that need to be fixed (https://ci.appveyor.com/project/PowerShell/xnetworking/build/3.1.567.0#L2707). There are two main ones:
    a. using WMI cmdlets instead of CIM - these can still be used, but it is recommended that you use CIM if you can. Are you able to convert the WMI calls to CIM calls? They are usually fairly similar. If this can't be done then let me know and the rule can be suppressed.
    b. using Alias cmdlets. In this case you're using Where instead of Where-Object. Just change all where cmdlets to where-object.
    c. You're missing a Write-Verbose call in Get-TargetResource - just put a verbose entry indicating that function in being called. See the other xNetworking resources for examples.

Finally, there is a test coverage issue that needs to be fixed - e.g. we need to increase unit test coverage by about 49 lines, but I can make some suggestions around that once the other stuff is done. I'll also do a proper code review as well and make some suggestions on that.

Thanks for all your help and I appreciate the work on this - it can be a bit of a steep learning curve but well worth it and I'm happy to help!

@PlagueHO PlagueHO added the waiting for author response The pull request is waiting for the author to respond to comments in the pull request. label Jun 24, 2017
@edycus
Copy link
Author

edycus commented Jul 11, 2017

Thanks you for the help. I see it is stilling failing one test. I can't see what the error is. What do I need to do next?

@edycus
Copy link
Author

edycus commented Jul 11, 2017

@PlagueHO what next?

@tysonjhayes
Copy link
Collaborator

@edycus - I'm not seeing a failing unit test. What I am seeing is a failure in the code coverage. You'll need to write unit tests for this in order for us to keep our code coverage up. As @PlagueHO noted though you have a few style violations, specifically as he called out in item 4.

Are you familiar with how to write unit tests or is this something we can help you get started?

@edycus
Copy link
Author

edycus commented Jul 12, 2017 via email

@PlagueHO
Copy link
Member

Hi @edycus - a unit test is a type of automated test that lets us validate that the code behaves as intended.

This is a good post that summarizes the different types of tests:
https://blogs.technet.microsoft.com/heyscriptingguy/2015/12/16/unit-testing-powershell-code-with-pester/

These unit tests run every time code is commited to this repo and validate that the code is working correctly and also helps prevent regression problems (changes that cause other unintended problems or bugs). We use a test framework for PowerShell called Pester to perform the testing (it makes it much easier).

You can find all the unit tests in files named \tests\unit\MSFT_*.tests.ps1. E.g. https://github.com/PowerShell/xNetworking/tree/dev/Tests/Unit

You'll want to create a unit test file for your resource - e.g \tests\unit\MSFT_xNetPowerManagement.tests.ps1 and add Pester tests to test the behavior of each *-TargetResource function.

Writing tests can be a little bit difficult, but it is a very useful skill to have (especially as the industry is talking about unit testing infrastructure using these frameworks). So it is worth persevering with. I can help you if you get stuck, but see if this info helps you make a start and we'll go from there!

Your work on this is really appreciated!

@edycus
Copy link
Author

edycus commented Jul 12, 2017 via email

@edycus
Copy link
Author

edycus commented Jul 28, 2017

@PlagueHO I have resubmitted. I defiantly need some help with the test side of this. I don't fully understand how to test. Do I need to download pester?

@PlagueHO
Copy link
Member

Hi @edycus - writing unit tests is definitely one of the hardest parts to creating DSC resource (and PowerShell in general), but an awesome skill worth having (some of our teams use Pester tests to validate servers are configured correctly and working as expected as well as testing code).

Anyway, you do need to have the Pester PowerShell module installed. E.g.

Install-Module Pester

I actually wrote a blog series a couple of years back on getting started with DSC contributions and it had a section on testing. Have a look at this page: https://dscottraynsford.wordpress.com/2015/12/18/creating-professional-dsc-resources-part-4/

I can also give you a hand constructing the tests as well. Have a look at the blog post and see if that explains anything in more detail.

@PlagueHO
Copy link
Member

One more thing: Are you using Visual Studio Code to edit in? You should - it's free and by far the best tool for working with PS in my opinion 😁

It'll allow you to auto-format your code to the style guidelines of the DSC Resource Kit automatically. Just open the file and press SHIFT+ALT+F and it'll correct much of the style issues (otherwise I'd need to comment on them to get you to fix them and I'm being lazy 😁 ). Make sure you've got the PowerShell extension installed into Visual Studio Code too (otherwise the SHIFT+ALT+F won't work).

@edycus
Copy link
Author

edycus commented Jul 28, 2017

@PlagueHO I am using Visual studio but the get-hub connection is a little foreign to me. If I go to File, Open , File and select MSFT_xNetPowerManagement.psm1. The only thing SHIFT+ALT+F does is open up my file menu at the top. LOL. What am I missing here?
2017-07-28_1518

@PlagueHO
Copy link
Member

@edycus - ahhhh! That's Visual Studio 2015 (or 2017 maybe). Visual Studio Code is different - it's a very lightweight cross platform IDE from Microsoft. It looks a bit different. Like this:
image

But you'd need the PS extension:
image

@edycus
Copy link
Author

edycus commented Jul 28, 2017

I see that I changed the code in my github site. Do I have to submit another pull request?

@edycus
Copy link
Author

edycus commented Jul 28, 2017

Never mind. I see it running now

@edycus
Copy link
Author

edycus commented Jul 28, 2017

I am taking off for today. Thanks for your help. The pieces are starting to fit together.

@johlju
Copy link
Member

johlju commented Jul 29, 2017

@edycus If you want to learn Pester, The Pester Book is very good source explaining it. I recommend it to anyone wanting to learn Pester.

@dsccommunity dsccommunity deleted a comment from msftclas Sep 27, 2017
@dsccommunity dsccommunity deleted a comment from msftclas Sep 27, 2017
@PlagueHO
Copy link
Member

PlagueHO commented Dec 9, 2017

Hi @edycus - just wondered if you needed any help on this one? I'm happy to give you a hand implementing the tests.

@edycus
Copy link
Author

edycus commented Dec 9, 2017 via email

@PlagueHO
Copy link
Member

PlagueHO commented Dec 9, 2017

@edycus - sure thing! What I could do is submit a PR to your dev branch with the tests. I'll take a look at doing this when I can (next few weeks if time permits).

@johlju
Copy link
Member

johlju commented May 23, 2018

Labeling this PR as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on the PR is taken up again.

@johlju johlju added abandoned The pull request has been abandoned. and removed waiting for author response The pull request is waiting for the author to respond to comments in the pull request. labels May 23, 2018
@PlagueHO
Copy link
Member

It appears I have still yet to submit some tests to your branch @edycus - so this is mostly waiting on me. I'll try and resuscitate this one when I have time.

@PlagueHO PlagueHO changed the base branch from dev to master July 11, 2020 06:29
@PlagueHO
Copy link
Member

Closing because abandoned

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned The pull request has been abandoned.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants