Skip to content

Conversation

@papadeltasierra
Copy link
Collaborator

Adding some unit tests.
REVIEWER

  • Are these valid tests?
  • Is this really the correct results from the Mocked methods?
  • Is there more testing that could/should be done?

Comment on lines -18 to -37
param (
[string]$ApiVersion = "2022-09-01"
)

# This is a known endpoint.
$MetadataEndpoint = "https://management.azure.com/metadata/endpoints?api-version=$ApiVersion"

try {
$Response = Invoke-RestMethod -Uri $MetadataEndpoint -Method Get -StatusCodeVariable StatusCode

if ($StatusCode -ne 200) {
$Msg = "ARM metadata endpoint '$MetadataEndpoint' returned status code $($StatusCode)."
throw $Msg
}
}
catch {
$Msg = "Failed to request ARM metadata $MetadataEndpoint."
Write-Error "$Msg Please ensure you have network connection. Error: $_"
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why removing this block?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it is replaced by the code below.


# Get the default config dataplane endpoint.
if (-not $ConfigDpEndpoint) {
if ($null -eq $ConfigDpEndpoint) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, is $null -eq a standard way?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invoke-ScriptAnalyzer prefers this way around. Not totally sure why but I do know in "C" that this was preferred incase you mistyped if (field == NULL) as if (field = NULL) - both compile but if (NULL = field) will not!

$chartLocationUrlSegment = "azure-arc-k8sagents/healthCheck?api-version=$apiVersion"
$chartLocationUrl = "$configDPEndpoint/$chartLocationUrlSegment"
$uriParameters = @{}
$uriParameters = [ordered]@{}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why ordered?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to be able to test and without "ordered" you cannot be sure what order the resulting URI string will have the parameters in.

$uriParameters = [ordered]@{}
Invoke-RestMethodWithUriParameters `
-Method "POST" `
-Uri "https://invalid.invalid/some/page/nowhere" `
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway to extract into a shared constant?

$configDpEndpoint | Should -Be "https://eastus2.dp.kubernetesconfiguration.azure.com"
}

# !!PDS: How do we validate this? Need to check endpoint on a sovereign cloud.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does sovereign login endpoint look like?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://login.chinacloudapi.cn/ for example but it appears that the az CLI assumes 3 URI components and only wants to extract the last one.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds like it's for mooncake and fairfax?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but do you have access to them? Not sure I do and the endpoints appear to return errors when I try. Perhaps they just don't have the update DP deployed yet though.

{
$loadEnvPath = Join-Path $PSScriptRoot 'loadEnv.ps1'
if (-Not (Test-Path -Path $loadEnvPath)) {
$loadEnvPath = Join-Path $PSScriptRoot '..\loadEnv.ps1'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A general question on UT, how to test those functions in helpers separately?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not figured that out yet but it would certainly be good if we could. I also don't like the current import process.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants