Skip to content
This repository has been archived by the owner on Feb 24, 2021. It is now read-only.

System Environment variable PSModulePath shouldn't be updated for unit tests #55

Closed
rchaganti opened this issue May 18, 2016 · 11 comments
Closed

Comments

@rchaganti
Copy link

The current resource test template updates the PSModulePath system variable for LCM to load the resource modules during integration tests. For unit tests, this is not necessary. Doing so and terminating a test without the test environment restore causes ISE to hang (while opening) when the added module path contains a folder that has multiple subfolders. For example, in of my tests, I had the resource modules for unit tests in C:\temp. The test preparation functions added C:\ to the PSModulePath system variable that caused ISE to scan the entire drive for modules.

cioe-utuoaaiysh

For integration tests too, this need not be done. Instead, the modules can be copied over the existing module path and cleaned up later.

@iainbrighton
Copy link

Aha! I wondered why this was happening. Gradually, it takes longer and longer to start a PowerShell session when autoloading modules - due to the hundreds of entries in the PSModulePath. Good catch :)

@PlagueHO
Copy link
Contributor

Thanks for logging this @rchaganti - appreciate it!

I totally agree, this is definitely a problem and needs to be fixed ASAP! Thanks for investigating it.

The original test methodolgy was to move the module to the existing PSModulePath folder, but this introduced it's own issues:

  1. If the same module already existed on the test machine in the PSModulePath then the existing version(s) would first have to be moved out to a temporary location before being tested. This becomes more complex when multiple different versions exist.
  2. If the unit tests were terminated by the user then the content of the PSModulePath would be left in an unknown state.

So reverting to the original process (moving the module toPSModuleRoot) might not be the ideal solution either. I'd say the path detection needs to be fixed primarily - so that in your case c:\Temp is set as the path rather than C:\ .

The existing Unit/Integration test templates are all wrapped in a try/catch/finally block with the finally block calling the Restore-TestEnvironment function. This was intended to preserve the environment no matter what occurred - if it isn't managing this then that needs to be fixed as well.

@tysonjhayes - have you got any thoughts on this one?

@PlagueHO
Copy link
Contributor

Looking at the code it seems to me that the problem might be that the detection method used to detect the root folder of the module could be at fault. Specifically one of these two lines:
https://github.com/PowerShell/DscResource.Tests/blob/dev/DscResourceTestHelper/TestHelper.psm1#L322
https://github.com/PowerShell/DscResource.Tests/blob/dev/DscResourceTestHelper/TestHelper.psm1#L325

@rchaganti - are you able to specify which Resource module you were testing?

@rchaganti
Copy link
Author

This happened while I was testing xVMNetworkAdapter resource that I pushed to xHyper-V.

@rchaganti
Copy link
Author

@PlagueHO why can't we simply rename the target folder if there is a name conflict and copy the new one that we need to run tests on? And, in the clean up code, we can restore the original name.

@rchaganti
Copy link
Author

This is for the PR I submitted in xHyper-V. The resource is xVMNetworkAdapter.

Get Outlook for Androidhttps://aka.ms/ghei36

On Wed, May 18, 2016 at 1:18 AM -0700, "Daniel Scott-Raynsford" <notifications@github.commailto:notifications@github.com> wrote:

Looking at the code it seems to me that the problem might be that the detection method used to detect the root folder of the module could be at fault. Specifically one of these two lines:
https://github.com/PowerShell/DscResource.Tests/blob/dev/DscResourceTestHelper/TestHelper.psm1#L322
https://github.com/PowerShell/DscResource.Tests/blob/dev/DscResourceTestHelper/TestHelper.psm1#L325

@rchagantihttps://github.com/rchaganti - are you able to specify which Resource module you were testing?

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHubhttps://github.com//issues/55#issuecomment-219957604

@KarolKaczmarek
Copy link
Contributor

@rchaganti
So your folder structure was
C:\temp\xHyper-v\DscResources\xVMNetworkAdapter ?
If that's the case then indeed we should have added C:\temp to the PSModulePath, not the C:\ ,so the culprit may be what @dcrreynolds pointed at.

Simply renaming the folder in case of the conflict won't work since if you interrupt tests we will be left with the renamed folder - we are already unwinding our changes to PSModulePath in the finally block and looks like it doesn't work the way we expect, that's another area to investigate though.

@PlagueHO
Copy link
Contributor

@rchaganti - I found the problem. Your Unit tests must be in a sub folder called Unit if you're using the standard test templates.

So you should have a path like this:
/Tests/Unit/MSFT_xVMNetworkAdapter.tests.ps1

See xNetworking for an example.

That said, the code should deal with this situation better and perhaps do a better job at detecting the root of the DSC module, so even if the tests are in a path that is not exepected the root would still be detected. This could be done by simply walking up the tree and looking for the right PSD1 file (because we know the name of the Resource module we're in already).

Also, no matter what happens the test code should always return the environment back to it's original state as @KarolKaczmarek mentioned. The only time it might not if it was terminated unexpectedly via a crash or killing the task. I have confirmed that using CTRL+C will still fire the Finally block and return the environment back to normal.

@PlagueHO
Copy link
Contributor

@KarolKaczmarek - is it worth me making the change I suggested? E.g. so that we locate the Root of the module by walking up the tree from the test file until we find the Resource Module filename?

I'm not sure what we can do about fixing the problem with the environment being reset though.

@KarolKaczmarek
Copy link
Contributor

@PlagueHO Yes please, we could improve the logic to find module root.

@PlagueHO
Copy link
Contributor

PlagueHO commented Jun 7, 2016

Cool - I'll get onto it.

KarolKaczmarek added a commit that referenced this issue Jul 22, 2016
…l-For-Unit-Tests

Only integration tests will change the Machine PSModulePath - Fixes #55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants