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

Refactor tests testmodules #297

Open
wants to merge 24 commits into
base: development
from

Conversation

@anamnavi
Copy link
Collaborator

@anamnavi anamnavi commented Oct 26, 2020

Refactored the Find tests to use test modules instead of production modules. Since the team has control and ownership over these test modules, testing for an exact version or anything else that is dependent upon the package not being updated is less error prone than before. This makes the tests more reliable as a failing test would indicate an error with the code, not dependent packages having changed. Resolves #263

@anamnavi anamnavi requested review from SteveL-MSFT and alerickson Oct 26, 2020
@anamnavi anamnavi self-assigned this Oct 28, 2020
$res = Find-PSResource -Name NonExistantCommand
$res | Should -BeNullOrEmpty
It "find Command resource given Name parameter" {
$res = Find-PSResource -Name "test_command_module"

This comment has been minimized.

@SteveL-MSFT

SteveL-MSFT Nov 2, 2020
Member

I think test modules should use the namespace: Microsoft.PowerShell.Test.*

This comment has been minimized.

@anamnavi

anamnavi Nov 17, 2020
Author Collaborator

I discussed this with Amber and her view was that we may be better off changing this when we move to NuGet gallery or reach the situation when we need the names to have the Microsoft namespace to stand out and not conflict with generic test modules. The way to change the name from something like test_module to Microsoft.PowerShell.Test.* is to either create a new module with this name or to update it in the database/from the backend which Amber could do. It's not a simple update by publishing the module with an updated version (with the name change made in). So her suggestion was to update once when we know for sure what we want it to be like.

test/FindResource.Command.Tests.ps1 Outdated Show resolved Hide resolved
test/FindResource.Command.Tests.ps1 Outdated Show resolved Hide resolved
test/FindResource.Command.Tests.ps1 Outdated Show resolved Hide resolved
test/FindResource.Module.Tests.ps1 Outdated Show resolved Hide resolved
test/FindResource.Module.Tests.ps1 Outdated Show resolved Hide resolved
test/FindResource.RoleCapability.Tests.ps1 Outdated Show resolved Hide resolved
test/FindResource.Script.Tests.ps1 Outdated Show resolved Hide resolved
test/testRepositories.xml Outdated Show resolved Hide resolved
@@ -1,6 +1,8 @@
<?xml version="1.0" encoding="utf-8"?>
<configuration>
<Repository Name="psgettestlocal" Url="file:///c:/code/testdir" Priority="50" Trusted="false" />

<Repository Name="psgettestlocal" Url="file:///c:/code/testdir" Priority="40" Trusted="false" />

This comment has been minimized.

@SteveL-MSFT

SteveL-MSFT Nov 2, 2020
Member

these file paths only work on Windows and only if that machine has C: drive which isn't guaranteed

This comment has been minimized.

@anamnavi

anamnavi Nov 17, 2020
Author Collaborator

Good point! My thought was that this filepath gets changed before it's ever used, but I see that it's better to not have to rely on that alone. Better to not introduce something that could cause an issue later or if misused. Additionally, this really helped my understanding of how PSGet registers the default repos and which ones. I removed this and instead register the test repo and unregister it in the PSGetTestUtils file now, with Join-Path to create the path.

@SteveL-MSFT SteveL-MSFT requested a review from JamesWTruher Nov 2, 2020
test/FindResource.Command.Tests.ps1 Outdated Show resolved Hide resolved
test/FindResource.Command.Tests.ps1 Outdated Show resolved Hide resolved
catch {}

$res | Should -BeNullOrEmpty
{Find-PSResource -Name "test_command_module" -Version $Version -Repository $TestGalleryName -ErrorAction Ignore} | Should -Throw -ExceptionType ([ArgumentException])

This comment has been minimized.

@SteveL-MSFT

SteveL-MSFT Nov 17, 2020
Member

Suggested change
{Find-PSResource -Name "test_command_module" -Version $Version -Repository $TestGalleryName -ErrorAction Ignore} | Should -Throw -ExceptionType ([ArgumentException])
{ Find-PSResource -Name "test_command_module" -Version $Version -Repository $TestGalleryName -ErrorAction Ignore } | Should -Throw -ExceptionType ([ArgumentException])

This comment has been minimized.

@SteveL-MSFT

SteveL-MSFT Nov 17, 2020
Member

Why is -ErrorAction Ignore used here?

test/FindResource.Command.Tests.ps1 Outdated Show resolved Hide resolved
Find-PSResource -Name "test_command_module" -Version "*" -Repository $TestGalleryName | ForEach-Object {
if($_.Name -eq "test_command_module") {
$expectedModules += $_.Name
}

This comment has been minimized.

@SteveL-MSFT

SteveL-MSFT Nov 17, 2020
Member

Is there a reason to filter out incorrect results that may be returned? Seems like this test should validate that each returned module has the right name

test/FindResource.DSCResource.Tests.ps1 Outdated Show resolved Hide resolved
) {
param($Version, $Description)

$res = Find-PSResource -Name "test_command_module" -Version $Version -Repository $TestGalleryName -ErrorAction Ignore

This comment has been minimized.

@SteveL-MSFT

SteveL-MSFT Nov 17, 2020
Member

I thought we were going to use namespaced test module names?

$res = Find-PSResource -ModuleName "test_module" -Version "*" -Repository $TestGalleryName
$res.Count | Should -Be 7
$expectedModules = @()
Find-PSResource -ModuleName "test_module" -Version "*" -Repository $TestGalleryName | ForEach-Object {

This comment has been minimized.

@SteveL-MSFT

SteveL-MSFT Nov 17, 2020
Member

This code is similar in several cases, you can use -TestCases with splatting to cover different parameter variations and use the same validation code. Something like:

It "... with parameter '<Parameter>'" -TestCases @(
  @{ Parameter = 'Name' }
  @{ Parameter = 'ModuleName' }
) {
  param($parameter)
  ...
  $parameters = @{
    $parameter = 'test_module'
    Version = '*'
    Repository = $TestGalleryName
  }

  Find-PSResource @parameters
@@ -30,6 +30,50 @@ $script:PSGalleryLocation = 'https://www.powershellgallery.com/api/v2'
$script:PoshTestGalleryName = 'PoshTestGallery'
$script:PostTestGalleryLocation = 'https://www.poshtestgallery.com/api/v2'

$script:command_test_module = @(
@{Name="test_command_module"; Version="2.5.0.0"; Repository="PoshTestGallery"; Description="this a test command resource of type module"},

This comment has been minimized.

@SteveL-MSFT

SteveL-MSFT Nov 17, 2020
Member

lots of repetition, so recommend storing repeated values in a variable in case they need to change in the future or reused

This comment has been minimized.

@anamnavi

anamnavi Nov 19, 2020
Author Collaborator

stored in variable.

Get-NewPSResourceRepositoryFile
Get-RegisterLocalRepos

This comment has been minimized.

@JamesWTruher

JamesWTruher Nov 17, 2020
Member

Suggested change
Get-RegisterLocalRepos
Register-LocalRepos
$res = Find-PSResource -Name Az.Compute
$res | Should -Not -BeNullOrEmpty
$res.Name | Should -Be "Az.Compute"
Get-UnregisterLocalRepos

This comment has been minimized.

@JamesWTruher

JamesWTruher Nov 17, 2020
Member

Suggested change
Get-UnregisterLocalRepos
Unregister-LocalRepos

Register and Unregister are approved verbs

anamnavi added 2 commits Nov 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.