Extensions commit mop-up by maryamariyan · Pull Request #34208 · dotnet/runtime

maryamariyan · 2020-03-27T19:50:01Z

I applied a git filter-branch on extensions repo considering the proper file/folder paths expected in runtime repo.
merged commits locally into my local runtime repo.
Then cherry picked the new commits on this runtime repo branch.

UPDATE:

This PR also does test projects' cleanups and sets up Logging.EventSource tests: 8f9d98e

@Tratcher

- add debug information for `Assert` failures in `MaximumOSVersionTest` - update src/TestingUtils/Microsoft.AspNetCore.Testing/test/MaximumOSVersionTest.cs - co-Authored-By: @Tratcher - try with VS2019 queues - skip `MaximumOSVersion` tests on .NET due to xunit issue - co-authored-by: @Tratcher Commit migrated from dotnet/extensions@26ff835

* Remove Serilog dependency in extensions * Add xunit logging for shutdown tests * Need to remove dependency on AspNetCore.Testing and remove DumpCollector Commit migrated from dotnet/extensions@540b4e8

Commit migrated from dotnet/extensions@6171c14

…x-logging-event-source Fix LoggingEventSource to handle null strings Commit migrated from dotnet/extensions@8d0fa05

Commit migrated from dotnet/extensions@5bcbaa2

…dotnet/extensions#2940) * Add environment var to allow disabling live reload in default builder * Switch to using hostingContext, rename flag to fit nomenclature. * Change flag to memory data source. Change content root default instead of runtime default. * Update config key to be hierarchical. Change await on positive case to be longer and cancel using the reload token. * Uncomment a development test case. * Apply suggestions from code review Co-authored-by: Chris Ross <Tratcher@Outlook.com> Commit migrated from dotnet/extensions@64140f9

maryamariyan · 2020-03-27T21:07:50Z

Filed an issue for the net472 failure here: #34216

##[error]src\libraries\Common\tests\Extensions\TestingUtils\Microsoft.AspNetCore.Testing\src\xunit\FrameworkSkipConditionAttribute.cs(53,8): error CS1029: #error: 'Target frameworks need to be updated.'

https://dev.azure.com/dnceng/public/_build/results?buildId=576706

PR: #34218

ericstj · 2020-03-30T16:39:33Z

Failure is due to number formatting issue in test:

-273.15 37
-273,15 37

JunTaoLuo · 2020-03-30T18:11:56Z

Yea somehow it's using the European format for the decimal. Hmm

JunTaoLuo · 2020-03-30T18:29:18Z

So the issue here is that the machines are setup with a non-US local so the format here: https://github.com/dotnet/runtime/blob/master/src/libraries/Microsoft.Extensions.Logging.EventSource/tests/EventSourceLoggerTest.cs#L723 uses a comma instead of a decimal point. I would check what format EventSource uses for doubles and use the same format in the test code. My assumption would be that they are using CultureInfo.InvariantCulture but I'm not sure.

maryamariyan · 2020-03-30T20:06:12Z

yes, the spanish locale is setup on the machines.

I get a repro on the test failures locally by changing the locale like below:

Assert.True(CultureInfo.CurrentCulture.Name.Equals("en-US", StringComparison.OrdinalIgnoreCase));
CultureInfo.CurrentCulture = new CultureInfo("es-ES");
Assert.True(CultureInfo.CurrentCulture.Name.Equals("es-ES", StringComparison.OrdinalIgnoreCase));
...

ericstj · 2020-03-31T00:49:13Z

InvariantCulture seems to be the right choice. I believe this is the code that is doing the formatting:

runtime/src/libraries/Microsoft.Extensions.Logging.Abstractions/src/LogValuesFormatter.cs

Lines 127 to 147 in b85563d

returnstring.Format(CultureInfo.InvariantCulture,_format,values??EmptyArray);
}
internalstringFormat()
{
return_format;
}
internalstringFormat(objectarg0)
{
returnstring.Format(CultureInfo.InvariantCulture,_format,FormatArgument(arg0));
}
internalstringFormat(objectarg0,objectarg1)
{
returnstring.Format(CultureInfo.InvariantCulture,_format,FormatArgument(arg0),FormatArgument(arg1));
}
internalstringFormat(objectarg0,objectarg1,objectarg2)
{
returnstring.Format(CultureInfo.InvariantCulture,_format,FormatArgument(arg0),FormatArgument(arg1),FormatArgument(arg2));

JunTaoLuo · 2020-03-31T03:02:26Z

nit: I see another double ToString() here. Should that be fixed here as well? If it's not causing failures it might be because it's not used, in which case why is it present?

I noticed them as well. but seems only this change should be sufficient

I’d fix them all for correctness. It’s not like we have a reason for using current culture

I'll do that in a future PR.

ericstj · 2020-03-31T04:23:34Z

Why here and not at line 154?

It has to go somewhere after line https://github.com/dotnet/runtime/blob/master/src/libraries/Directory.Build.props#L238 otherwise the property would get reassigned with MicrosoftShared by:
https://github.com/dotnet/arcade/blob/master/src/Microsoft.DotNet.Arcade.Sdk/tools/ProjectDefaults.props#L6

I see, so line 154 does nothing. 🤦‍♂ We should file a bug on that. I wonder if that caused other projects to get a different public key.

I don't think other projects have a problem, because other projects have each their own Directory.Build.props next to their solution files which specifies the StrongNameKeyId. e.g.:
https://github.com/dotnet/runtime/blob/master/src/libraries/System.Globalization/Directory.Build.props#L4
https://github.com/dotnet/runtime/blob/master/src/libraries/System.Data.Odbc/Directory.Build.props#L4

But the intent of that line was to make the default for corefx (at the time) to be the Open key. If it’s not having any effect that means we may have some projects which relied on it but no longer get it. IOW they didn’t set Open key in their d.b.p. As it stands it does nothing and should be deleted.

- Updates InternalsVisibleTo files as well - Setup Logging.EventSource.Tests - Fix three failing EventSourceLoggerTest tests - Delete file: IntegrationTesting csproj not needed. - Rename folder test -> tests (Logging/tests/DI.Common/tests) - Cleanup other csproj fix compile issue

maryamariyan · 2020-03-31T05:00:29Z

squashed last three commits into one and keeping rest as they were mop up commits

dougbuand others added 6 commits March 27, 2020 12:37

Remove Serilog dependency in extensions (dotnet/extensions#3040)
0b8f8bc
* Remove Serilog dependency in extensions * Add xunit logging for shutdown tests * Need to remove dependency on AspNetCore.Testing and remove DumpCollector Commit migrated from dotnet/extensions@540b4e8

Add more tests
4cd7907
Commit migrated from dotnet/extensions@6171c14

Merge pull requestdotnet/extensions#2957from sywhang/dev/suwhang/fi…
9f8fdb7
…x-logging-event-source Fix LoggingEventSource to handle null strings Commit migrated from dotnet/extensions@8d0fa05

Fixed variable name typo (dotnet/extensions#3085)
4ad8f3d
Commit migrated from dotnet/extensions@5bcbaa2

maryamariyan added the area-Microsoft.Extensions label Mar 27, 2020

maryamariyan mentioned this pull request Mar 27, 2020
mono test failure: System.Security.Cryptography.Csp.Tests #34222
Closed

maryamariyan self-assigned this Mar 28, 2020

JunTaoLuo reviewed Mar 31, 2020
View reviewed changes

JunTaoLuo approved these changes Mar 31, 2020
View reviewed changes

ericstj reviewed Mar 31, 2020
View reviewed changes

ericstj approved these changes Mar 31, 2020
View reviewed changes

maryamariyan force-pushed the inspection branch from 732b7e5 to 87e7991Compare March 31, 2020 04:59

maryamariyan merged commit 96a3bfe into dotnet:masterMar 31, 2020

ericstj mentioned this pull request Apr 2, 2020
Move StrongNameKeyId=Open default value down after arcade is imported #34428
Merged

maryamariyan mentioned this pull request Jun 12, 2020
Remove workarounds and cleanup source to meet dotnet/runtime analyzer requirements. #37821
Closed
11 tasks

maryamariyan mentioned this pull request Sep 27, 2020
Minor cleanup on Extensions in Configuration #42788
Merged
1 task

maryamariyan added area-Meta and removed area-Microsoft.Extensions labels Nov 5, 2020

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020

Uh oh!

Conversation

maryamariyan commented Mar 27, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maryamariyan commented Mar 27, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericstj commented Mar 30, 2020

Uh oh!

JunTaoLuo commented Mar 30, 2020

Uh oh!

JunTaoLuo commented Mar 30, 2020

Uh oh!

maryamariyan commented Mar 30, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ericstj commented Mar 31, 2020

Uh oh!

JunTaoLuoMar 31, 2020

Choose a reason for hiding this comment

Uh oh!

maryamariyanMar 31, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericstjMar 31, 2020

Choose a reason for hiding this comment

Uh oh!

maryamariyanMar 31, 2020

Choose a reason for hiding this comment

Uh oh!

ericstjMar 31, 2020

Choose a reason for hiding this comment

Uh oh!

maryamariyanMar 31, 2020

Choose a reason for hiding this comment

Uh oh!

ericstjMar 31, 2020

Choose a reason for hiding this comment

Uh oh!

maryamariyanMar 31, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericstjMar 31, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maryamariyan commented Mar 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

maryamariyan commented Mar 27, 2020•
edited
Loading

maryamariyan commented Mar 27, 2020•
edited
Loading

maryamariyan commented Mar 30, 2020•
edited
Loading

maryamariyanMar 31, 2020•
edited
Loading

maryamariyanMar 31, 2020•
edited
Loading

ericstjMar 31, 2020•
edited
Loading