-
Notifications
You must be signed in to change notification settings - Fork 246
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
Write unit tests for build and sign release #1521
base: main
Are you sure you want to change the base?
Conversation
# 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a note to my code stating that this might be coming.
BeforeDiscovery { | ||
$ScriptPath = Join-Path -Path $PSScriptRoot -ChildPath '../../utils/workflow/Install-AzureSignTool.ps1' -Resolve | ||
# Source the function | ||
. $ScriptPath | ||
Install-AzureSignTool | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
🗣 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