-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Fix unit tests and the AOT compatibility. #53627
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
base: feature/ai-foundry/agents-v2
Are you sure you want to change the base?
Conversation
sdk/ai/Azure.AI.Agents/src/Custom/Internal/AdditionalPropertyHelpers.cs
Outdated
Show resolved
Hide resolved
70f9c17 to
f7f0ed9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Packages changes look good.
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
f7f0ed9 to
476a07a
Compare
…re/azure-sdk-for-net into nirovins/fix_unit_tests
| newRequestWriter.Write("Content-Type: application/octet-stream\r\n"); | ||
| } | ||
| newRequestWriter.WriteLine(line); | ||
| newRequestWriter.Write($"{line}\r\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it the case that Write is AOT-compatible and WriteLine isn't? That's so strange!!
I'd normally suggest that we should use Environment.NewLine instead of an explicit \r\n, but I have a sneaking suspicion that Environment.Newline resolution might be exactly why WriteLine doesn't work -- is that the case? newRequestWriter.Write(line).Write(Environment.NewLine) is "cleaner," but not if reproduces the same break!
Functionally, as long as this still works across platforms (which should be caught by SDK tests), I have no issue with the explicit \r\n there.
0f19b1e to
e5b55d6
Compare
|
|
||
| <!-- TEMP: Overrides until central packages bumped, needed for new extensions --> | ||
| <PackageReference Include="Microsoft.Bcl.AsyncInterfaces" VersionOverride="8.0.0" /> | ||
| <PackageReference Include="Microsoft.Bcl.AsyncInterfaces" VersionOverride="9.0.9" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ArthurMa1978 / @ArcturusZhang: We're not going to address it in this PR, but we should NOT be doing version overrides like this. Most of these are behind the central versions at this point. Please follow-up and remove these.
Contributing to the Azure SDK
Please see our CONTRIBUTING.md if you are not familiar with contributing to this repository or have questions.
For specific information about pull request etiquette and best practices, see this section.