test(amqp): use pinned CFSClean broker - #5023
Open
Johnathan W (j7nw4r) wants to merge 14 commits into
Open
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 2 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). 2 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
Restores broker-backed AMQP tests using a pinned CFSClean-compatible broker and prevents silent CI skips.
Changes:
- Pins, restores, starts, probes, and cleans up the AMQP test broker.
- Requires broker availability in non-macOS CI jobs.
- Centralizes broker-test gating and updates documentation.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
Test-Setup.ps1 |
Implements broker provisioning and readiness checks. |
Test-Cleanup.ps1 |
Cleans up active broker jobs. |
src/test_broker.rs |
Centralizes broker-test gating. |
src/lib.rs |
Registers the test helper module. |
src/connection.rs |
Migrates connection tests to the helper. |
README.md |
Documents broker setup and pinning. |
nuget.cfsclean.config |
Pins the permitted NuGet feed. |
ci.tests.yml |
Requires the broker in non-macOS CI. |
Suppressed comments (1)
sdk/core/azure_core_amqp/README.md:80
- The manual CFSClean restore points at
azure-amqp\nuget.cfsclean.config, but that file does not exist in the pinned clone; this PR intentionally adds the config besideTest-Setup.ps1. Aftercd azure-amqp, this command therefore fails before restoring. Resolve and pass the config from the azure-sdk-for-rust checkout instead.
dotnet restore .\test\TestAmqpBroker\TestAmqpBroker.csproj --configfile .\nuget.cfsclean.config
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Johnathan W (j7nw4r)
force-pushed
the
j7nw4r/amqp-cfsclean-broker
branch
from
August 10, 2026 21:37
dd9f08b to
4816284
Compare
Larry Osterman (LarryOsterman)
approved these changes
Aug 10, 2026
Heath Stewart (heaths)
left a comment
Heath Stewart (heaths)
left a comment
Member
There was a problem hiding this comment.
This should just rely on the root nuget.config or it may cause conflicts otherwise.
Pin broker setup to the azure-amqp commit that owns the CFSClean NuGet config. Restore through that config, build net10.0, and fail with broker output when readiness does not arrive. Check that the clone stays clean, re-enable cleanup, and document the pin plus the external developer and CFSClean build paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Teach cspell the broker config file and option names used by the CFSClean setup path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The broker tests skipped when TEST_BROKER_ADDRESS was absent, and they still passed. The suite therefore stayed green when the broker did not run at all. Add a test_broker helper. TEST_BROKER_REQUIRED makes a missing broker an error, and the pipeline sets it for every job except macOS, where setup never starts a broker. Run the restore and the build from the clone root. The dotnet command reads global.json from the current directory and not from the project directory, so the earlier absolute paths lost the SDK pin of the clone. Stop the broker from the finally block behind a success flag. Invoke-LoggedCommand calls exit from inside itself, so the earlier inline calls did not run on every failure path and left the broker on port 25672. Bound each readiness attempt. The blocking Connect method takes no timeout, so a dropped SYN made the probe run far past its own budget. Add TEST_BROKER_COMMIT to override the pinned commit without a code change, and test whether the pin is reachable from master through the GitHub compare API. An unreachable pin warns, and TEST_BROKER_REQUIRE_MERGED turns that warning into an error. A check that cannot run always continues, because the anonymous rate limit is shared and says nothing about the pin.
A non-numeric ahead_by read as "not reachable" before. A build that sets TEST_BROKER_REQUIRE_MERGED could then fail because of a malformed answer and not because of a bad pin. Count that case as "the check did not run" instead. Coerce after the absence test and never instead of it, because $null -as [int] gives 0. A coercion on its own would turn a missing ahead_by field into 0 and report an unreachable pin as reachable. Correct the comment above the pin. The SHA is now an ancestor of refs/pull/318/head and not the head itself, because that pull request has moved on.
The previous commit wrapped the rev-parse call in "$( ... )" to take the last output line. That broke the argument. The outer double-quoted string consumes the escaped quotes first, so the command split into three arguments. The second one bound to -ExecutePath, and git ran with a bare -C. CI showed the result: > git -C Push-Location: Cannot find path '.../TestArtifacts/azure-amqp rev-parse HEAD' because it does not exist. Use two statements instead, so no outer string can consume the quotes. The last-line behavior stays the same, and the result is still a string.
REMOVE BEFORE MERGE. Report the package sources that the pipeline agent resolves from inside the broker clone. No local test can answer whether the agent turns off the source named nuget.org, which feeds the agent allows, or whether the explicit config narrows the list on a real agent. The block prints and does nothing else. It restores the error preference around itself, so it cannot fail the run.
This reverts commit 00a8c23.
The restricted feed policy is this pipeline's requirement, and not a requirement of Azure/azure-amqp. The configuration that satisfies a policy belongs with the code that enforces the policy, so this repository now carries its own nuget.cfsclean.config next to the setup script, and the restore passes that file. Drop the check for a restore config inside the broker clone, and check the copy in this repository instead. The pinned commit no longer has to contain a restore config, so the pin can move to any commit that builds the broker.
Azure/azure-amqp pull request 318 closed without a merge, so its head commit is not on any branch and can disappear. This repository now owns the restore config, so the pin no longer has to reach a commit that carries one, and any commit that builds the broker works. Pin the head of master. The reachability check stays quiet, because that commit is reachable from master.
The cspell step failed on nuget.cfsclean.config with "Unknown word (configfile)". Say the same thing in plain words instead of adding a dictionary entry.
The helper had one test, and it covered an absent variable that no rule depends on. The behavior that keeps a silent skip from returning was untested. Move the rule into resolve_broker_address, which reads no environment variable, and test every branch: a present address comes back, an absent address skips, and an absent address panics when the run requires a broker. Cover the documented flag values too. Correct the pin update steps and the README. They said the pinned commit must contain nuget.cfsclean.config. This package owns that file now, so any commit that builds the broker works.
TEST_BROKER_REQUIRED sat in eng/pipelines/templates/jobs/ci.tests.yml, which every package uses for its tests. The flag belongs to this package, so the template no longer carries it and this change leaves that file untouched. Test-Setup.ps1 sets the flag instead, right after the macOS guard. That placement gives the macOS behavior for free, because the script exits before this line on macOS and the flag never gets set there. The script sets the process variable and also emits the pipeline logging command, so the value reaches a later step as well. Move the spelling words into .cspell.json for this package, and drop the inline cspell directives from the README and the two scripts.
The package carried its own nuget.cfsclean.config, and that file was a copy of eng/templates/NuGet.config.template, which the pipeline already uses through eng/pipelines/templates/steps/nuget-config.yml. Two files with the same contents drift apart. Delete the copy and name the template on the restore. The restore still has to name a configuration. The broker clone carries its own nuget.config that adds NuGet.org, and a directory level file wins over the user level one that the pipeline writes. Keep the pinned commit in Test-Setup.ps1 alone. README.md repeated the SHA, so an update needed two edits and could leave the two out of step. The README now points at the script.
Johnathan W (j7nw4r)
force-pushed
the
j7nw4r/amqp-cfsclean-broker
branch
from
August 11, 2026 13:14
ab7742c to
44ce2b2
Compare
Johnathan W (j7nw4r)
enabled auto-merge (squash)
August 11, 2026 13:33
The pin update steps still told the reader to write the SHA into README.md. Test-Setup.ps1 is the only place that holds the pin now, and the paragraph below the steps already says so. The two statements disagreed.
Heath Stewart (heaths)
requested changes
Aug 12, 2026
| # adds NuGet.org, and a directory level file wins over the user level one, so the restore | ||
| # has to name a configuration explicitly. Pass an absolute path, because the dotnet calls | ||
| # run from the clone root. | ||
| $nugetConfig = [System.IO.Path]::Combine($RepoRoot, "eng", "templates", "NuGet.config.template") |
There was a problem hiding this comment.
This needs to wait until Daniel Jurek (@danieljurek) gets his in. Daniel, can you make sure the feed here is in that nuget.confg as well?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Restores the broker backed
azure_core_amqptests, and makes a missing broker fail the build instead of passing it.Motivation
Work item 39130334 requires the Rust AMQP tests to run without changing any file in the broker clone.
The setup script exited at its first line, so the broker never started. The tests then took a skip branch and still passed, and the suite reported the same counts with the broker and without it. Nothing turned red, so the loss stayed invisible.
Changes
nuget.cfsclean.configto this repository and restores the broker through it. The feed policy is this pipeline's requirement, so the configuration that satisfies it lives with the pipeline and not in the broker clone.global.jsonfrom the current directory and not from the project directory.src/test_broker.rs.TEST_BROKER_REQUIREDmakes an absent broker an error instead of a skip, and the CI template sets it for every job except macOS, where the setup script never starts a broker.finallyblock behind a success flag.Invoke-LoggedCommandcallsexitfrom inside itself, so the earlier inline calls left a broker on port 25672 on some failure paths.Connectmethod takes no timeout, so a dropped SYN made the probe run far past its own budget.TEST_BROKER_COMMITto retarget the broker, and a GitHub compare API check that warns when the pin is not reachable frommaster.TEST_BROKER_REQUIRE_MERGEDmakes that an error. A check that cannot run always continues.Validation
cargo test -p azure_core_amqp --all-featureswith no broker and no flag: the suite passes and the broker tests skip. WithTEST_BROKER_REQUIREDset and no broker: the suite fails and names the setup script.cargo fmtandcargo clippy -D warningsare clean.