Skip to content

[DurableTask.ServiceBus.Tests] Fix GetHubQueueMaxDeliveryCountsAsync throwing when a hub queue is missing - #1423

Closed
Davide Montanari (davidemontanari) wants to merge 2 commits into
Azure:mainfrom
davidemontanari:davidemontanari/dtfx-sb-test-fix-maxdeliverycount-get
Closed

Davide Montanari (davidemontanari) wants to merge 2 commits into
Azure:mainfrom
davidemontanari:davidemontanari/dtfx-sb-test-fix-maxdeliverycount-get

Conversation

@davidemontanari

Copy link
Copy Markdown
Member

Summary

Two small cleanups. Neither changes production behaviour: GetHubQueueMaxDeliveryCountsAsync is internal and only used by the integration tests in TaskHubClientTests.

1. Missing queues now return -1 instead of throwing

GetHubQueueMaxDeliveryCountsAsync used Single(...) to find the orchestrator, worker and tracking queues. When a queue was missing, Single threw InvalidOperationException. Because of that, the existing ?.MaxDeliveryCount ?? -1 fallback could never run.

  • Replaced Single with SingleOrDefault, so a missing queue returns -1 as the fallback intended. In TestMaxDeliveryCount and TestMaxDeliveryCountIfNew, a missing queue now shows up as a clear assertion failure instead of an unrelated exception.
  • Moved the lookup into an internal GetHubQueueMaxDeliveryCounts(IEnumerable<QueueDescription>) method so it can be unit tested without a Service Bus namespace. The async method still fetches the queues and delegates to it.

2. Removed connection strings with keys from offline tests

WaitForOrchestrationTests used a fake connection string containing SharedAccessKey=.... Fake as it is, it looks like a secret and can trip secret scanners.

  • Added NoOpTokenCredential, a shared test helper. It is a TokenCredential stub that never returns a token, plus a placeholder host name.
  • Offline tests now build ServiceBusOrchestrationService with the host name and TokenCredential constructor instead of a connection string.

Tests

  • Added HubQueueMaxDeliveryCountsTests, which runs offline using ServiceBusModelFactory:
    • All three queues present: each returns its max delivery count.
    • Tracking queue missing: tracking returns -1 and the others keep their values.
    • Only another hub's queues present: all three return -1.
  • With the original Single, 2 of the 3 new tests fail; with the fix, all 3 pass.
  • HubQueueMaxDeliveryCountsTests and WaitForOrchestrationTests pass on net8.0 and net48 (24 tests).

Copilot AI balanced review requested due to automatic review settings October 22, 2026 05:44
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The uppercase Test/ paths exclude both new files from the test project on case-sensitive checkouts, breaking compilation and preventing regression-test discovery.

1 open issue
What changed in this PR

This PR fixes missing-queue handling in the Service Bus provider’s internal test helper and removes a secret-like connection string from offline tests.

Changes:

  • Extracts queue-count lookup and uses SingleOrDefault so missing queues return -1.
  • Adds three offline regression tests for queue-count lookup.
  • Introduces a shared token credential stub for offline service construction.
File Description
test/​DurableTask.ServiceBus.Tests/​WaitForOrchestrationTests.cs Replaces the fake connection string with a credential stub.
Test/​DurableTask.ServiceBus.Tests/​NoOpTokenCredential.cs Adds the shared offline credential helper.
Test/​DurableTask.ServiceBus.Tests/​HubQueueMaxDeliveryCountsTests.cs Adds coverage for present and missing hub queues.
src/​DurableTask.ServiceBus/​ServiceBusOrchestrationService.cs Extracts lookup logic and enables the missing-queue fallback.

Review effort: Balanced

Comment thread test/DurableTask.ServiceBus.Tests/NoOpTokenCredential.cs
Copilot AI balanced review requested due to automatic review settings October 22, 2026 05:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The focused changes implement the intended fallback with targeted regression coverage and no identified blocking issues; tests were not run during this review.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants