Split Azure.Iot.Sdk.Test into a versioned module with checks - #450
Conversation
scripts/Azure.Iot.Sdk.Test.psm1 was a single 3984-line, 170 KB file holding
nine unrelated concerns, with no manifest, no version and no tests, imported by
pipelines in other repositories.
Move it to scripts/AzIotSdkTest/ as a manifested module:
AzIotSdkTest.psd1 ModuleVersion, exported surface, PowerShellVersion 5.1
AzIotSdkTest.psm1 root module
Import-Parts.ps1 load order and Export-ModuleMember
parts/*.ps1 Common, AzureCommon, Conversion, Crypto, Models, Dps,
ResourceGroups, Provisioning, TestConfig, SubmoduleGraph
No code changed: concatenating the header, the parts in load order and the
export list reproduces the previous file byte for byte, and the exported
commands compare identical to master's, name by name and parameter by
parameter, including parameter types.
The load order is explicit rather than a directory listing, because PowerShell
resolves a class used as a parameter type when the file declaring the consumer
is parsed: Models.ps1 must load before anything taking a [TestEnvironmentInfo].
The loader is a .ps1 so the deprecated path can dot-source it; the dot-source
operator accepts only .ps1. scripts/Azure.Iot.Sdk.Test.psm1 stays as a shim
that dot-sources the loader from its checkout, and downloads the repository
when it was itself downloaded standalone -- which is how consumers that fetch
that single file from raw.githubusercontent.com keep working unchanged. Its
default ref is master, matching what those consumers get today. The shim goes
away once they move to a pinned checkout.
tests/Validate-Module.ps1 parses every part, fails when parts/ and the load
order disagree in either direction, imports the module and asserts the exported
set matches both the declared contract and the manifest, and checks the shim
still yields the same commands. It provisions nothing and needs no credentials.
CI runs it on ubuntu with pwsh and on windows with Windows PowerShell 5.1,
which is what AzureCLI@2 scriptType 'ps' gives the Azure Pipelines templates.
The two Azure Pipelines templates and the architecture doc now import the
manifest.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`shell: ${{ matrix.shell }}` is evaluated before the matrix context exists, so
the workflow was rejected as a workflow file error and no job ran at all.
Select the shell with two steps guarded on runner.os instead, which keeps the
Windows leg running Windows PowerShell 5.1 -- the version AzureCLI@2
scriptType 'ps' gives the Azure Pipelines templates.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate findings affect shim cleanup and validation of exported command signatures and surfaces.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Splits the Azure IoT SDK test framework into a versioned PowerShell module while preserving the legacy import path.
Changes:
- Adds an explicit manifest, loader, exports, and implementation parts.
- Updates pipelines and documentation to use the new module.
- Adds validation CI and retains standalone shim compatibility.
File summaries
| File | Reviewed changes |
|---|---|
vsts/templates/steps-create-azure-resources.yaml |
Imports the new manifest. |
vsts/templates/steps-cleanup-leftover-resources.yaml |
Imports the new manifest. |
tests/Validate-Module.ps1 |
Validates module structure, exports, and shim behavior. |
scripts/AzIotSdkTest/README.md |
Documents module usage and layout. |
scripts/AzIotSdkTest/parts/TestConfig.ps1 |
Split framework implementation. |
scripts/AzIotSdkTest/parts/SubmoduleGraph.ps1 |
Split framework implementation. |
scripts/AzIotSdkTest/parts/ResourceGroups.ps1 |
Split framework implementation. |
scripts/AzIotSdkTest/parts/Provisioning.ps1 |
Split framework implementation. |
scripts/AzIotSdkTest/parts/Models.ps1 |
Split framework implementation. |
scripts/AzIotSdkTest/parts/Dps.ps1 |
Split framework implementation. |
scripts/AzIotSdkTest/parts/Crypto.ps1 |
Split framework implementation. |
scripts/AzIotSdkTest/parts/Conversion.ps1 |
Split framework implementation. |
scripts/AzIotSdkTest/parts/Common.ps1 |
Split framework implementation. |
scripts/AzIotSdkTest/parts/AzureCommon.ps1 |
Split framework implementation. |
scripts/AzIotSdkTest/Import-Parts.ps1 |
Defines load order and exports. |
scripts/AzIotSdkTest/AzIotSdkTest.psm1 |
Root module loader. |
scripts/AzIotSdkTest/AzIotSdkTest.psd1 |
Defines version and exported surface. |
docs/horton-architecture.md |
Documents the new module path. |
.gitignore |
Ignores validation dependencies. |
.github/workflows/validate-module.yml |
Runs cross-platform module validation. |
Review details
Suppressed comments (5)
scripts/AzIotSdkTest/Import-Parts.ps1:23
- This comment has an article agreement error: “an provisioned” should be “a provisioned.”
'Models.ps1' # Classes describing an provisioned test environment
scripts/AzIotSdkTest/README.md:38
- This example points to an action path that does not exist in this repository (there is no
actions/directory), so following it cannot provide the checkout or import the module. Show a checkout of this repository at the pinned tag instead.
* GitHub Actions: `uses: Azure/iot-sdks-e2e-fx/actions/<action>@<tag>`
tests/Validate-Module.ps1:134
- The shim validation is asymmetric: it reports missing expected commands but never rejects an unexpected exported function. A shim that adds a public command would therefore pass even though this change promises the exact 16-command surface; add the reverse comparison as is already done for the manifest.
$ShimExports = Get-ExportedNames -Path $ShimPath
foreach ($Name in @($ExpectedExports | Where-Object { $ShimExports -notcontains $_ })) {
$Problems.Add("The scripts/Azure.Iot.Sdk.Test.psm1 shim no longer yields '$Name'.")
}
tests/Validate-Module.ps1:97
- These branches only inspect the current checkout; the script never creates an orphan-part or missing-listed-part fixture. Consequently the verification description's claimed negative tests are not actually exercised in CI. Add isolated fixture-based checks or remove that claim from the verification description.
foreach ($Missing in @($PartFiles.Name | Where-Object { $Listed -notcontains $_ })) {
$Problems.Add("parts/$Missing exists but is not listed in `$ModuleParts, so it is never loaded.")
}
foreach ($Extra in @($Listed | Where-Object { $PartFiles.Name -notcontains $_ })) {
$Problems.Add("`$ModuleParts lists '$Extra', which does not exist under parts/.")
}
tests/Validate-Module.ps1:106
- This validation only collects
ExportedFunctions.Keys, so it checks command names but never compares parameter metadata (including parameter types) or verifies that the shim and manifest have identical signatures. A future split could change a public parameter while this check still passes, despite the PR's stated name-and-parameter compatibility guarantee. Add a signature comparison for each exported command, including the shim.
$Module = Import-Module $Path -Force -PassThru
try {
return @($Module.ExportedFunctions.Keys | Sort-Object)
- Files reviewed: 19/21 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Raised in review: the standalone-import path created a unique staging directory per import and never removed it, so every raw-file consumer left an archive and an extracted checkout behind on the agent, including on a failed download. Wrap the download, extraction and dot-source in try/finally and remove the directory recursively in the finally. Dot-sourcing parses the files at that point, so nothing under the staging directory is needed once it returns. Verified against this branch: the module still exports all 16 commands and they remain callable after the directory is gone, and both the success and the failure path leave no staging directory behind. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The split preserves the original implementation byte-for-byte and validates module loading, exports, and PowerShell 5.1 compatibility.
Review details
- Files reviewed: 19/21 changed files
- Comments generated: 0 new
- Review effort level: Balanced
The composite actions landed on master importing scripts/Azure.Iot.Sdk.Test.psm1, which this branch turns into a deprecated shim for consumers that download that single file. Callers inside this repository have the checkout, so they import the manifest directly and the shim is left to the external raw-URL consumers it exists for. This matches what the Azure Pipelines templates in this branch already do. AZ_IOT_MODULE now points at scripts/AzIotSdkTest/AzIotSdkTest.psd1 in both actions that import it, and the config-cmdlet guard matches ModuleName 'AzIotSdkTest' -- importing by manifest names the module after the manifest, so the previous string would have rejected every renderer. tests/Validate-ActionScripts.ps1 loads the manifest for the same reason, and the validate-actions workflow now triggers on scripts/** rather than only the shim file. Verified on the merged tree: both validators pass; all 6 config renderers are accepted and New-AzureResourceGroupName, New-AzIotTestEnvironment and a wildcard are rejected; Test-SubmoduleConsistency resolves; and the misspelled cmdlet, splat drift and script-body expression checks all still fire. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Updated for #449, now merged.
Verified on the merged tree: both validators pass; all 6 config renderers accepted and |
Raised in review: the shim check only asserted that every expected command is present, so a command exported by the shim alone would have passed despite the stated requirement that both paths expose the same set. Compare both ways, as the manifest check already does. Verified by adding an extra export to the shim: the check now fails naming it, and passes again once removed. Also: "an provisioned" -> "a provisioned" in the Models.ps1 load-order comment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
scripts/Azure.Iot.Sdk.Test.psm1was a single 3984-line, 170 KB file holding nine unrelated concerns, with no manifest, no version and no tests, imported by pipelines in other repositories.Move it to
scripts/AzIotSdkTest/:AzIotSdkTest.psd1ModuleVersion1.0.0, exported surface,PowerShellVersion 5.1AzIotSdkTest.psm1Import-Parts.ps1Export-ModuleMemberparts/*.ps1No code changed. Concatenating the header, the parts in load order and the export list reproduces the previous file byte for byte.
The load order is explicit rather than a directory listing: PowerShell resolves a class used as a parameter type when the file declaring the consumer is parsed, so
Models.ps1must load before anything taking a[TestEnvironmentInfo].The loader is a
.ps1so the deprecated path can dot-source it — the dot-source operator accepts only.ps1.scripts/Azure.Iot.Sdk.Test.psm1stays as a shim that dot-sources the loader from its checkout, and downloads the repository when it was itself downloaded standalone, which is how consumers fetching that single file keep working unchanged. Its default ref ismaster, matching what those consumers get today. The shim goes away once they move to a pinned checkout.The two Azure Pipelines templates and
docs/horton-architecture.mdnow import the manifest.Verification
Run locally, all passing:
Get-AzureResourceGroupNamePrefix,New-AzureResourceGroupName, a class + crypto path, and the module-scope$DefaultCertificateExpiration): all match master.tests/Validate-Module.ps1:OK: 10 parts, 16 exported commands, shim intact.CI runs the checks on ubuntu with pwsh and on windows with Windows PowerShell 5.1, which is what
AzureCLI@2scriptType: psgives the Azure Pipelines templates.Not verified: an actual provisioning run, which needs an Azure subscription and a service principal.
Follow-ups (not in this PR)
TestConfig.ps1, which build the same variable list twice (once asexport X=, once as$env:X =).SubmoduleGraph.ps1(791 lines) into its own module; it shares only two helpers with the rest.