Extensions commit mop-up #34208

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

    
               return string.Format(CultureInfo.InvariantCulture, _format, values ?? EmptyArray); 
        
           } 
        
           internal string Format() 
        
           { 
        
               return _format; 
        
           } 
        
           internal string Format(object arg0) 
        
           { 
        
               return string.Format(CultureInfo.InvariantCulture, _format, FormatArgument(arg0)); 
        
           } 
        
           internal string Format(object arg0, object arg1) 
        
           { 
        
               return string.Format(CultureInfo.InvariantCulture, _format, FormatArgument(arg0), FormatArgument(arg1)); 
        
           } 
        
           internal string Format(object arg0, object arg1, object arg2) 
        
           { 
        
               return string.Format(CultureInfo.InvariantCulture, _format, FormatArgument(arg0), FormatArgument(arg1), FormatArgument(arg2));

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

src/libraries/Microsoft.Extensions.Logging.EventSource/tests/EventSourceLoggerTest.cs

@@ -720,7 +810,7 @@ private static class EventTypes
            { "E6MSG", (e) => VerifySingleEvent(e, "Logger2", EventTypes.Message, 6, null, LogLevel.Warning) },

            { "E7FM", (e) => VerifySingleEvent(e, "Logger3", EventTypes.FormattedMessage, 7, null, LogLevel.Information,
-                @"""FormattedMessage"":""Logger3 Event7 Information inner scope closed " + DoubleParam2.ToString() + " 37") },
+                @"""FormattedMessage"":""Logger3 Event7 Information inner scope closed " + DoubleParam2.ToString("G", CultureInfo.InvariantCulture) + " 37") },
            { "E7JS", (e) => VerifySingleEvent(e, "Logger3", EventTypes.MessageJson, 7, null, LogLevel.Information,
                        @"""ArgumentsJson"":{""stringParam"":""inner scope closed"",""doubleParam"":""" + DoubleParam2.ToString() + @""",""intParam"":""37""") },


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

src/libraries/Directory.Build.props

@@ -245,6 +244,9 @@
    <!-- Always pass portable to override arcade sdk which uses embedded for local builds -->
    <DebugType>portable</DebugType>

+    <!-- Microsoft.Extensions projects have a separate StrongNameKeyId -->
+    <StrongNameKeyId Condition="$(MSBuildProjectName.StartsWith('Microsoft.Extensions.'))">MicrosoftAspNetCore</StrongNameKeyId>


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

dougbu and 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 request dotnet/extensions#2957 from 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 87e7991 Compare March 31, 2020 04:59

maryamariyan merged commit 96a3bfe into dotnet:master Mar 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

Extensions commit mop-up #34208

Extensions commit mop-up #34208

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!

JunTaoLuo Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

maryamariyan Mar 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!

ericstj Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

maryamariyan Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

ericstj Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

maryamariyan Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

ericstj Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

maryamariyan Mar 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!

ericstj Mar 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!

Uh oh!

maryamariyan commented Mar 27, 2020 •

edited

Loading

maryamariyan commented Mar 27, 2020 •

edited

Loading

maryamariyan commented Mar 30, 2020 •

edited

Loading

maryamariyan Mar 31, 2020 •

edited

Loading

maryamariyan Mar 31, 2020 •

edited

Loading

ericstj Mar 31, 2020 •

edited

Loading