-
Notifications
You must be signed in to change notification settings - Fork 5.1k
[Azure.Core.Amqp] Fix AmqpMessageBodyTests #22008
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
Conversation
The `DateTime` and `DateTimeOffset` test values in `AmqpMessageBodyTests.s_amqpValues` were created with `DateTimeOffset.Parse`, which would fail with the following exception when run on a system with a non-US culture:
```
System.TypeInitializationException : The type initializer for 'Azure.Core.Amqp.Tests.AmqpMessageBodyTests' threw an exception.
----> System.FormatException : String was not recognized as a valid DateTime.
at Azure.Core.Amqp.Tests.AmqpMessageBodyTests..ctor()
--FormatException
at System.DateTimeParse.Parse(String s, DateTimeFormatInfo dtfi, DateTimeStyles styles, TimeSpan& offset)
at System.DateTimeOffset.Parse(String input)
at Azure.Core.Amqp.Tests.AmqpMessageBodyTests..cctor()
```
By using `new DateTimeOffset(2021, 3, 24, 0, 0, 0, TimeSpan.Zero)` instead of `DateTimeOffset.Parse("3/24/21")` we ensure that the tests pass on any system regardless of its configured culture.
|
Thank you for your contribution @0xced! We will review the pull request and get back to you soon. |
jsquire
left a comment
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.
Hi @0xced. Thank you for your contribution and for helping to improve the Azure SDK experience. The change looks good to me.
The CI failures that we're seeing appear to be related to a missing file in another area. Would you be so kind as to try rebasing your changes onto the latest from main and see if that resolves?
|
@JoshLove-msft: Would you mind taking a look as well? I think the CI failures may be related to one of the recent Event Grid changes... |
The latest commit on main (cbad4bb) is from three days ago and I just did this pull request today on top of this commit so there's nothing new I can rebase on. Maybe another branch needs to be merged on |
|
Created #22012 |
|
/azp run net - core - ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
The
DateTimeandDateTimeOffsettest values inAmqpMessageBodyTests.s_amqpValueswere created withDateTimeOffset.Parse, which would fail with the following exception when run on a system with a non-US culture:By using
new DateTimeOffset(2021, 3, 24, 0, 0, 0, TimeSpan.Zero)instead ofDateTimeOffset.Parse("3/24/21")we ensure that the tests pass on any system regardless of its configured culture.