Refactor PSWSIISEndpoint.psm1 to meet Style Guidelines and Best Practices - Githubissues

dsccommunity / xPSDesiredStateConfiguration

DSC resources for configuring common operating systems features, files and settings.

https://dsccommunity.org

MIT License

206 stars 132 forks source link

Refactor PSWSIISEndpoint.psm1 to meet Style Guidelines and Best Practices #518

Open PlagueHO opened 5 years ago

PlagueHO commented 5 years ago

The DSCResources/MSFT_xDSCWebService/PSWSIISEndpoint.psm1 module should be refactored to bring it in line with DSC Resource Kit Style Guidelines and Best Practices.

This includes the following tasks (creation of tests should be prioritized):

[ ] Create unit tests for all functions
[ ] Create integration tests
[ ] Fix Style Guideline violations
[ ] Fix Best Practice violations
[ ] Improve coding standards such as expressive function names, clean code principles, adoption of SRP (Single Responsibility Principle)

tmeckel commented 5 years ago

Im working on this right now because I'm trying to get my PR https://github.com/PowerShell/xPSDesiredStateConfiguration/pull/507 finally out the door.

tmeckel commented 5 years ago

@PlagueHO @mhendric can we align the work on the MSFT_xDSCWebService.psm1 in some way?

mhendric commented 5 years ago

Hey @tmeckel . You may want to wait until #506 is merged before you start working on this one, as it corrects a bunch (but not all) of the existing style and best practice violations. Also, I'm working on Integration tests for MSFT_xDSCWebService.psm1 now (#476). I'm hoping to have a PR out for review later today. We may want to wait for that one to be approved and merged before we do any more major work on this resource. Or maybe focus on the Unit tests or something since that will need new code.

mhendric commented 5 years ago

Alright, #506 has been merged. You're probably free to work on the style and best practice violations now. And again, I'm hoping to have a PR with the new integration tests in later today.

tmeckel commented 5 years ago

Hi @mhendric I rebased my PR https://github.com/PowerShell/xPSDesiredStateConfiguration/pull/507 (surprisingly) without any collisions.

mhendric commented 5 years ago

Cool. I got #525 submitted to add Integration tests (although it needs code reviews).

tmeckel commented 5 years ago

@mhendric finally my PR https://github.com/PowerShell/xPSDesiredStateConfiguration/pull/507 got successfully tested by Appveyor. But the code coverage looks odd!!

mhendric commented 5 years ago

@tmeckel , it's possible you're hitting #480. Still not sure what causes that, but we do need to investigate sooner or later.

tmeckel commented 5 years ago

@mhendric, okay... Perhaps I'm lucky and it's only the issue you mentioned. Anyway... I do need to make some integration tests in the customers environment with the new code.

mhendric commented 5 years ago

FWIW, the actual code coverage on both your Unit test runs look fine to me:

Code coverage report: Covered 73.90 % of 4,901 analyzed Commands in 28 Files.

mhendric commented 5 years ago

Since you mentioned Integration tests, I think it's worth calling out that we probably don't need integration tests for PSWSIISEndpoint.psm1 itself. If we add the correct set of Integration tests for MSFT_xDSCWebService, that should indirectly test PSWSIISEndpoint.psm1. Let me know if you guys disagree though.

tmeckel commented 5 years ago

@mhendric I agree with you that we don't need specific integration tests for PSWSIISEndpoint.psm1 because it's a submodule to MSFT_xDSCWebService. But I think we do need some (more?) Unit tests which ensure the proper (inner) working of PSWSIISEndpoint.psm1.

mhendric commented 5 years ago

Hey @tmeckel , absolutely, we could definitely use Unit tests to cover as much as possible (ideally 100%) of PSWSIISEndpoint.psm1 directly. I think we have 0% code coverage right now.