Repository navigation
Conversation
|
/AzurePipelines run |
There was a problem hiding this comment.
Pull request overview
This PR updates Microsoft.OData.Client async entry points to avoid APM (Begin/End + blocking waits) and instead use native Task-based async, improving Blazor WebAssembly compatibility while expanding regression coverage to ensure sync/APM response paths aren’t used.
Changes:
- Replaced remaining
FromAsync(Begin…, End…)wrappers with native async implementations for save/batch/paging/stream/bulk/deep-insert APIs. - Updated
DataServiceActionQuerySingle<T>.GetValueAsyncto useExecuteAsyncand refreshed unit tests accordingly. - Expanded WASM-focused unit tests and updated functional test request-message mocks to support
GetResponseAsync.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/Microsoft.OData.Client/DataServiceContext.cs |
Converts multiple public async APIs to native async implementations (save/batch/load property/read stream/bulk/deep insert/paging). |
src/Microsoft.OData.Client/DataServiceActionQuerySingleOfT.cs |
Switches action single-result async execution to ExecuteAsync and preserves nullable/non-nullable semantics. |
test/UnitTests/Microsoft.OData.Client.Tests/Serialization/AsyncWasmCompatibilityTests.cs |
Adds broader WASM regression coverage and enforces “no sync/APM response APIs” in the test request message. |
test/UnitTests/Microsoft.OData.Client.Tests/DataServiceActionQuerySingleTests.cs |
Updates tests to validate the new native async execution path and cancellation token flow. |
test/FunctionalTests/Tests/DataServices/UnitTests/Client.TDD.Tests/Tests/DeepInsertE2ETests.cs |
Updates mock request message to support GetResponseAsync. |
test/FunctionalTests/Tests/DataServices/UnitTests/Client.TDD.Tests/Tests/BulkUpdateE2ETests.cs |
Updates mock request message to support GetResponseAsync. |
Suppressed comments (2)
src/Microsoft.OData.Client/DataServiceContext.cs:2282
- BulkUpdateAsync now passes a hard-coded method name ("BulkUpdateAsync") into BulkUpdateSaveResult. The BulkUpdate/BeginBulkUpdate paths use Util.BulkUpdateMethodName; using the same constant keeps method-name-based diagnostics consistent and avoids duplicated strings.
BulkUpdateSaveResult result = new BulkUpdateSaveResult(this, "BulkUpdateAsync", SaveChangesOptions.BulkUpdate, null, null);
await result.BulkUpdateRequestAsync(cancellationToken, objects).ConfigureAwait(false);
src/Microsoft.OData.Client/DataServiceContext.cs:2355
- DeepInsertAsync has inconsistent modifier ordering ("public async virtual" vs the rest of this type's "public virtual async") and also hard-codes the method name ("DeepInsertAsync") when constructing DeepInsertSaveResult. Aligning both with existing conventions (and Util.DeepInsertMethodName) keeps the codebase consistent and preserves method-name-based diagnostics.
public async virtual Task<DataServiceResponse> DeepInsertAsync<T>(T resource, CancellationToken cancellationToken)
{
if (resource == null)
{
throw Error.ArgumentNull(nameof(resource));
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
/AzurePipelines run |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
test/EndToEndTests/Tests/Client/Microsoft.OData.Client.E2E.Tests/CancellationTokenTests/Tests/CancellationTokenTests.cs:91
- Avoid asserting on the exact exception type/message for cancellation.
TaskCanceledExceptionvsOperationCanceledExceptionand the exception message are runtime/localization-dependent; asserting on them makes this E2E test brittle.
var exception2 = await Assert.ThrowsAsync<TaskCanceledException>(response2);
Assert.Equal("A task was canceled.", exception2.Message);
test/EndToEndTests/Tests/Client/Microsoft.OData.Client.E2E.Tests/CancellationTokenTests/Tests/CancellationTokenTests.cs:212
- Avoid asserting on the exact exception type/message for cancellation.
TaskCanceledExceptionvsOperationCanceledExceptionand the exception message are runtime/localization-dependent; asserting on them makes this E2E test brittle.
var exception = await Assert.ThrowsAsync<TaskCanceledException>(response);
Assert.Equal("A task was canceled.", exception.Message);
test/EndToEndTests/Tests/Client/Microsoft.OData.Client.E2E.Tests/CancellationTokenTests/Tests/CancellationTokenTests.cs:295
- Avoid asserting on the exact exception type/message for cancellation.
TaskCanceledExceptionvsOperationCanceledExceptionand the exception message are runtime/localization-dependent; asserting on them makes this E2E test brittle.
var exception = await Assert.ThrowsAsync<TaskCanceledException>(response);
Assert.Equal("A task was canceled.", exception.Message);
test/EndToEndTests/Tests/Client/Microsoft.OData.Client.E2E.Tests/CancellationTokenTests/Tests/CancellationTokenTests.cs:226
- Avoid asserting on the exact exception type/message for cancellation.
TaskCanceledExceptionvsOperationCanceledExceptionand the exception message are runtime/localization-dependent; asserting on them makes this E2E test brittle.
var exception2 = await Assert.ThrowsAsync<TaskCanceledException>(response2);
Assert.Equal("A task was canceled.", exception2.Message);
test/EndToEndTests/Tests/Client/Microsoft.OData.Client.E2E.Tests/CancellationTokenTests/Tests/CancellationTokenTests.cs:231
- Avoid asserting on the exact exception type/message for cancellation.
TaskCanceledExceptionvsOperationCanceledExceptionand the exception message are runtime/localization-dependent; asserting on them makes this E2E test brittle.
var exception3 = await Assert.ThrowsAsync<TaskCanceledException>(response3);
Assert.Equal("A task was canceled.", exception3.Message);
| var exception = await Assert.ThrowsAsync<TaskCanceledException>(response); | ||
| Assert.Equal("A task was canceled.", exception.Message); |
|
I built this branch locally (head Why the current tests don't show it
ReproductionI reused the existing
All four fail with Non-batch: Batch: Root cause
Stream contentStream = await response.Content.ReadAsStreamAsync().ConfigureAwait(false);
return new HttpWebResponseMessage(…, getResponseStream: () => contentStream, getResponseStreamAsync: null);The query path copes because Possible fixes
Either way it would help to have a response stream in the tests that rejects synchronous reads. #3538 already introduces exactly such a helper ( Two smaller notes
|
Summary
Tests
Fixes #3605