Write unit tests for build and sign release #1521

james-garriss · 2025-01-17T19:50:23Z

🗣 Description

Moved some of the build and sign release into a function. Reused existing unit tests and wrote a new one.

Note: Testing Azure Sign Tool itself should be a functional test that is added to nightly tests. A new issue was created for this (#1520).

💭 Motivation and context

Closes: #1453

🧪 Testing

Ran workflow pipeline, checked the results.

✅ Pre-approval checklist

✅ Pre-merge checklist

PR passed smoke test check.
Feature branch has been rebased against changes from parent branch, as needed

Use Rebase branch button below or use this reference to rebase from the command line.
Resolved all merge conflicts on branch
Notified merge coordinator that PR is ready for merge via comment mention
Demonstrate changes to the team for questions and comments.
(Note: Only required for issues of size Medium or larger)

✅ Post-merge checklist

Feature branch deleted after merge to clean up repository.
Verified that all checks pass on parent branch (e.g., main or release-name) after merge.

buidav · 2025-01-20T17:43:44Z

.github/workflows/build_sign_release.yaml

+      # Note: This is NOT the ACTUAL release version for ScubaGear.
+      # That value is found in ScubaGear.psd1.
+      # This is only used for things like the release file name.
+      # Yes, this is a disconnect that violates DRY.
      version:
        description: "Release Version (e.g., 1.2.4)"
        required: true


I think the workflows are now in a position where we could refactor out having to using a slimmer version of this function to pull out the version number from the manifest. Future enhancement of course.

I like the concept, and I'm willing to use it, but I don't like the implementation. If pulling values from a "config file" requires a regex, we're doing it wrong. Surely there's a smarter way!? Or a different type of "config file" could be used for such values?

Well, whatever the implementation, I recommend creating an issue to do this work in the future.

I added a note to my code stating that this might be coming.

buidav · 2025-01-20T20:18:54Z

Testing/workflow/Install-AzureSignTool.Tests.ps1

+BeforeDiscovery {
+  $ScriptPath = Join-Path -Path $PSScriptRoot -ChildPath '../../utils/workflow/Install-AzureSignTool.ps1' -Resolve
+  # Source the function
+  . $ScriptPath
+  Install-AzureSignTool
+}


A question.
Is this unit test ever meant to be run locally?

If yes, then would it beneficial to check if the AzureSignInTool is already installed before attempting to do another installation?
If no, disregard question.

Functions that are in /utils are intended to be used and reused by any code. Functions that are in /utils/workflow, however, are only intended to be used by workflows. As we would not expect AST to already be installed on the runner, there doesn't seem to be a need to add a Pester test for that. If we moved this function up into /utils, then I think your idea would be smart.

Can you think of any other (non-workflow) code that would want to use the Install-AzureSignTool function?

schrolla

Looks good functionally, but recommend some style and comment updates. See below.

.github/workflows/publish_private_package.yaml

.github/workflows/publish_public_package.yaml

Testing/workflow/Build-SignRelease.Tests.ps1

Testing/workflow/Install-AzureSignTool.Tests.ps1

Testing/workflow/Build-SignRelease.Tests.ps1

schrolla · 2025-01-23T22:21:27Z

.github/workflows/publish_private_package.yaml

-          dotnet --version
-          dotnet tool install --global AzureSignTool --version 5.0.0
-      # OIDC Login to Azure Public Cloud with AzPowershell (enableAzPSSession true)
+          # Source the function


This comment is unnecessary as it just restates in plain english what the next line does using standard PowerShell syntax.

More pain and suffering.

Comment still present. Recommend removing entirely.

.github/workflows/build_sign_release.yaml

schrolla · 2025-01-23T22:24:10Z

utils/workflow/Build-SignRelease.ps1

+	param(
+		[Parameter(Mandatory = $true)]
+		[string]
+		$AzureKeyVaultUrl,
+		[Parameter(Mandatory = $true)]
+		[string]
+		$CertificateName,
+    [Parameter(Mandatory = $true)]
+    [string]
+    $ReleaseVersion,
+    [Parameter(Mandatory = $true)]
+    [string]
+    $RootFolderName
+	)


Recommend aligning parameters at same indent level for style consistency.

Suggested change

param(

[Parameter(Mandatory = $true)]

[string]

$AzureKeyVaultUrl,

[Parameter(Mandatory = $true)]

[string]

$CertificateName,

[Parameter(Mandatory = $true)]

[string]

$ReleaseVersion,

[Parameter(Mandatory = $true)]

[string]

$RootFolderName

)

param(

[Parameter(Mandatory = $true)]

[string]

$AzureKeyVaultUrl,

[Parameter(Mandatory = $true)]

[string]

$CertificateName,

[Parameter(Mandatory = $true)]

[string]

$ReleaseVersion,

[Parameter(Mandatory = $true)]

[string]

$RootFolderName

)

I am confused by this one, b/c they already are aligned. Maybe you are just seeing some weird formatting on GitHub?

@james-garriss It's because the first two params use tabs and the latter use spaces. You're IDE lines them up because you have tabstops set to 2, but GitHub has it set to 2. For consistency with style guide, recommend changing so all of the parameters (and all indents) convert tabs to spaces.

schrolla

Functionally all good. Saw one missed comment removal and convert tabs to spaces in one spot. But nothing to hold things up. Would recommend addressing before merge though.

schrolla · 2025-01-29T18:38:04Z

.github/workflows/publish_private_package.yaml

-          dotnet --version
-          dotnet tool install --global AzureSignTool --version 5.0.0
-      # OIDC Login to Azure Public Cloud with AzPowershell (enableAzPSSession true)
+          # Source the function


Comment still present. Recommend removing entirely.

james-garriss added 30 commits December 16, 2024 14:22

move sign code into function

5ba46c8

fix lint

021a9d8

fix path

43da682

fix path

9cd0bb1

fix path

aa00717

remove commented out code

31f5706

run test release method

1e35e16

Add debugging

6bc8651

Add temp pester test

db57541

test file

b21b053

add more debug

ee43e3c

debug path

a9bfb11

use copy instead of move

348bf9f

remove debug

6699668

Back to move

73ce101

restore

703f157

create Pester test for installing AST

6676866

install before checking for install

7250133

verify commands

426f4b2

Spell toolpath with a L

2f900fd

Spell $true as $true

55a2a38

remove debug

84ed7c8

fix lint

09e5c71

test for dotnet

23d0b07

Replace env

1c02f37

test azure login

9580ab2

Remove dead code, test copy folder again

046f12d

Restore code for a test

0822608

Restore login

41391b4

restore env

e42dddc

james-garriss added 14 commits January 8, 2025 12:27

change root folder name

bba4576

Test for root

46b0155

add resolve

dd40f78

fix root folder name

220f6ad

Throw exception for missing dir

932bca3

added function doc

96cca59

remove the remove-item

a4cb084

set root to repo

7c2c843

add remove-item back

e786cbd

clarify comments

c99f5b7

Cleanup

28dfeb5

add exception test

c0ec264

add scriptblock

2568231

Comments

430a3c1

james-garriss requested review from schrolla and buidav January 17, 2025 19:50

james-garriss self-assigned this Jan 17, 2025

james-garriss added enhancement This issue or pull request will add new or improve existing functionality infrastructure Related to configuring infrastructure necessary for the project labels Jan 17, 2025

remove outdated comments

cfb9af2

buidav reviewed Jan 20, 2025

View reviewed changes

james-garriss added 2 commits January 21, 2025 07:56

Add comment

c5a09b5

Lint

a6ecb8d

schrolla requested changes Jan 23, 2025

View reviewed changes

Remove helpful comment

a377b12

james-garriss requested a review from schrolla January 27, 2025 15:33

comment

decd14b

schrolla approved these changes Jan 29, 2025

View reviewed changes

james-garriss added this to the Lionfish milestone Jan 30, 2025

james-garriss requested a review from buidav January 30, 2025 18:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Write unit tests for build and sign release #1521

Write unit tests for build and sign release #1521

james-garriss commented Jan 17, 2025

buidav Jan 20, 2025

james-garriss Jan 21, 2025

james-garriss Jan 21, 2025

buidav Jan 20, 2025

james-garriss Jan 21, 2025

schrolla left a comment

schrolla Jan 23, 2025

james-garriss Jan 27, 2025

schrolla Jan 29, 2025

schrolla Jan 23, 2025

james-garriss Jan 27, 2025

schrolla Jan 29, 2025

schrolla left a comment

schrolla Jan 29, 2025

Write unit tests for build and sign release #1521

Are you sure you want to change the base?

Write unit tests for build and sign release #1521

Conversation

james-garriss commented Jan 17, 2025

🗣 Description

💭 Motivation and context

🧪 Testing

✅ Pre-approval checklist

✅ Pre-merge checklist

✅ Post-merge checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schrolla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schrolla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment