Skip to content
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

Fix LogRecordTest by using InvariantCulture. #1683

Merged

Conversation

Oberon00
Copy link
Member

@Oberon00 Oberon00 commented Jan 4, 2021

Fixes #1678.

Changes

Enforce that the current Culture is InvariantCulture around the call to ToString on the log record state.

Note that using String.Format(CultureInfo.InvariantCulture, "{0}", state) does not work here.

The test would fail if the current locale used something other than a
dot as the decimal separator (e.g. comma).
@Oberon00 Oberon00 requested a review from a team January 4, 2021 09:53
@codecov
Copy link

codecov bot commented Jan 4, 2021

Codecov Report

Merging #1683 (2aed8c3) into master (71290dc) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1683   +/-   ##
=======================================
  Coverage   82.09%   82.09%           
=======================================
  Files         249      249           
  Lines        6730     6730           
=======================================
  Hits         5525     5525           
  Misses       1205     1205           

@Oberon00
Copy link
Member Author

Same as #1680: Can we merge this yet? I cannot give you access to my branch since it's an organization repository unfortunately.

@cijothomas cijothomas merged commit 3bb0445 into open-telemetry:master Jan 11, 2021
@Oberon00 Oberon00 deleted the bugfix/logrecord-test branch January 11, 2021 17:44
@@ -213,7 +214,16 @@ public void CheckStateForStrucutredLogWithGeneralType()
Assert.Contains(state, item => item.Key == "{OriginalFormat}");
Assert.Equal("{food}", state.First(item => item.Key == "{OriginalFormat}").Value);

Assert.Equal("[Name, truffle], [Price, 299.99]", state.ToString());
var prevCulture = CultureInfo.CurrentCulture;
Copy link
Member

@reyang reyang Jan 11, 2021

Choose a reason for hiding this comment

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

If there is a common need, consider making this a "using statement".

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

This was the only test that failed that way, at least with a German locale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants