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

feat: Enable skip of CI deployment tests #4203

Open
wants to merge 49 commits into
base: main
Choose a base branch
from

Conversation

ReneHezser
Copy link
Contributor

@ReneHezser ReneHezser commented Jan 17, 2025

Description

Allow testcases to be excluded from deplyoments by the CI deployment.

Azure/Azure-Verified-Modules#1796

Pipeline Reference

Pipeline
avm.ptn.deployment-script.import-image-to-acr
avm.res.compute.gallery (intended)

Type of Change

  • Update to CI Environment or utilities (Non-module affecting changes)
  • Azure Verified Module updates:
    • Bugfix containing backwards-compatible bug fixes, and I have NOT bumped the MAJOR or MINOR version in version.json:
      • Someone has opened a bug report issue, and I have included "Closes #{bug_report_issue_number}" in the PR description.
      • The bug was found by the module author, and no one has opened an issue to report it yet.
    • Feature update backwards compatible feature updates, and I have bumped the MINOR version in version.json.
    • Breaking changes and I have bumped the MAJOR version in version.json.
    • Update to documentation

Checklist

  • I'm sure there are no other open Pull Requests for the same update/change
  • I have run Set-AVMModule locally to generate the supporting module files.
  • My corresponding pipelines / checks run clean and green without any errors or warnings

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Triage 🔍 Maintainers need to triage still Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue labels Jan 17, 2025
@ReneHezser ReneHezser added Type: CI 🚀 This issue is related to the AVM CI github_actions Pull requests that update GitHub Actions code and removed Needs: Triage 🔍 Maintainers need to triage still labels Jan 17, 2025
@@ -180,3 +180,4 @@
/avm/res/web/static-site/ @Azure/avm-res-web-staticsite-module-owners-bicep @Azure/avm-module-reviewers-bicep
/avm/utl/types/avm-common-types/ @Azure/avm-utl-types-avmcommontypes-module-owners-bicep @Azure/avm-module-reviewers-bicep
*avm.core.team.tests.ps1 @Azure/avm-core-team-technical-bicep
*.e2eignore @Azure/avm-core-team-technical-bicep
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking forward to discussing, great work meanwhile!
The main suggestion I'd have for now is to add tests. At least one pipeline run with one deployment test skipped and one base case pipeline with no test skipped

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @eriqua.
I am not sure I understand your remark. You mean we should have a failed test before the test can be excluded?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @ReneHezser exactly, suggested full test coverage for CI updates before they are introduced.

  • Static validation: we introduced 2 pester tests so we need:
    • module run with no test skipped --> should pass
    • module run where deployment tests are skipped as intended--> should pass
    • module run with one test skipped incorrectly (.e2eignore added in defaults or waf-aligned test) --> should fail pester test 1
    • module run with one test skipped incorrectly (.e2eignore is empty) --> should fail pester test 2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @eriqua
I added testcases for the mentioned scenarios. Before merging, I need to remove the tests again.

Copy link
Contributor

@eriqua eriqua Mar 1, 2025

Choose a reason for hiding this comment

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

Hey @ReneHezser, thanks. To show that the Pester test fails as intended, we need a link to the corresponding failing pipeline(s). Could you please attach a link to the failing pipeline runs?

Copy link
Contributor Author

@ReneHezser ReneHezser Mar 4, 2025

Choose a reason for hiding this comment

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

I thought I already did, but probably forgot to save the update on the description. Sorry.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry but still don't see them. Looking for specific pipeline runs, validating the four cases above, not pipeline badges.

@ReneHezser ReneHezser requested review from a team as code owners February 26, 2025 13:01
@avm-team-linter avm-team-linter bot added the Needs: Core Team 🧞 This item needs the AVM Core Team to review it label Feb 26, 2025
[string] $moduleFolderPath
)

$e2eTestFolderPathList = Get-ChildItem -Directory (Join-Path -Path $moduleFolderPath 'tests' 'e2e') | Where-Object { $_.Name -in 'defaults', 'waf-aligned' }
Copy link
Contributor

@AlexanderSehr AlexanderSehr Mar 1, 2025

Choose a reason for hiding this comment

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

Not sure if that's a good check. Our convention is not that the folders must be called defaults, but that there must be a test folder that ends with that in the name (same for waf).
You can find an example for this in e.g., the res/compute/virtual-machine module.
Also there the test in this file calls this out

"Module should contain a [tests/e2e/*defaults] folder."

A fix would be as easy as to thest that the name matches either defaults/waf-aligned (you could check for both in the same regex if you want).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I understand that some other test already failed over this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we may want to also have the same logic for the waf-aligned test? I.e., -like '*waf-aligned'?

Comment on lines 244 to 248
$passCiFilePath = Join-Path $moduleTestFolderPath '.e2eignore'
if (Test-Path $passCiFilePath) {
Write-Output "File '.e2eignore' exists in the folder: $moduleTestFolderPath"
# end here, as the test should be bypassed
return
Copy link
Contributor

Choose a reason for hiding this comment

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

If we must have it in here, please make it conditional via a switch statement the user must enable.
Where I to run tests locally, this would prevent me from running the test case even if I wanted to and would anyways only make sense if the user did not specify a single test file. Would feel very odd if I specifically provide a test file that happens to have the .e2eignore file in the folder (which may not work in the CI/AVM subscription, but may in mine), yet I cannot test it with this script 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

foreach ($e2eTestFolderPath in $e2eTestFolderPathList) {
$filePath = Join-Path -Path $e2eTestFolderPath '.e2eignore'
if (Test-Path $filePath) {
$incorrectParameters += $e2eTestFolderPath.Name
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it's more like 'incorrectFolders', not 'incorrectParameters'

if ($pathExisting) {
$fileContent = Get-Content -Path $filePath
if (-not $fileContent) {
$incorrectParameters += $e2eTestFolderPath.Name + '\.e2eignore'
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

@@ -234,8 +235,18 @@ function Test-ModuleLocally {
# -------------------------
if ((Get-Item -Path $ModuleTestFilePath) -is [System.IO.DirectoryInfo]) {
$moduleTestFiles = (Get-ChildItem -Path $ModuleTestFilePath -File).FullName
$moduleTestFolderPath = $ModuleTestFilePath
Copy link
Contributor

Choose a reason for hiding this comment

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

Beware, moduleTestFiles is an array in this case and this logic should not work. Did you validate how this behaves? Your logic would need to run below in the loops across the test files.

@@ -19,7 +19,7 @@ Optional. A string array that can be specified to run only Pester tests with the
Optional. A switch parameter that triggers a Pester test for the module

.PARAMETER ValidateOrDeployParameters
Copy link
Contributor

@AlexanderSehr AlexanderSehr Mar 4, 2025

Choose a reason for hiding this comment

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

Ideally, we don't inject the new condition into this object as it represents the parameters that are passed into the Test-TemplateDeployment & New-TemplateDeployment scripts (i.e., we want to be as close to the workflow as possible). I'd instead recommend an additional parameter. For example,

[Parameter(Mandatory = $false)]
[switch] $RespectE2eIgnoreFlag,

Copy link
Contributor

Choose a reason for hiding this comment

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

Still very much an active comment 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update GitHub Actions code Needs: Core Team 🧞 This item needs the AVM Core Team to review it Type: AVM 🅰️ ✌️ Ⓜ️ This is an AVM related issue Type: CI 🚀 This issue is related to the AVM CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants