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.

Sign up for GitHub

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

Jump to bottom

Enable remaining Extensions projects #33984

Merged
merged 9 commits into from
Mar 26, 2020
Merged

Enable remaining Extensions projects #33984

merged 9 commits into from
Mar 26, 2020

Conversation

maryamariyan
Copy link
Member

@maryamariyan maryamariyan commented Mar 23, 2020 •
edited
Loading

  • ref\src\test\pkg projects
  • need to review all pkgproj again to see if SupportedFrameworks are correct
  • tests enabled

Out of scope for this PR:

  • Refactor and include Hosting Functional and TestApp
  • Add sln files

@maryamariyan maryamariyan mentioned this pull request Mar 23, 2020
Closed
ericstj
ericstj reviewed Mar 24, 2020
View reviewed changes
...s/Microsoft.Extensions.Configuration.Json/src/Microsoft.Extensions.Configuration.Json.csproj Outdated Show resolved Hide resolved
...libraries/Microsoft.Extensions.Configuration.UserSecrets/tests/ConfigurationExtensionTest.cs
@@ -85,12 +85,12 @@ public void AddUserSecrets_ThrowsIfAssemblyAttributeFromType()
{
var ex = Assert.Throws<InvalidOperationException>(() =>
new ConfigurationBuilder().AddUserSecrets<string>());
Assert.Equal(Resources.FormatError_Missing_UserSecretsIdAttribute(typeof(string).GetTypeInfo().Assembly.GetName().Name),
Assert.Equal(SR.Format(SR.Error_Missing_UserSecretsIdAttribute, typeof(string).GetTypeInfo().Assembly.GetName().Name),
Copy link
Member

Choose a reason for hiding this comment

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

nit: existing, but this looks like it leaks the value of System.Private.CoreLib. And provides a pretty useless error message: https://github.com/dotnet/extensions/blob/5054f8a0e544b3f9e670808fd0f581d8ea03bc5f/src/Configuration/Config.UserSecrets/src/Resources.resx#L127-L129
Given a user cannot change System.Private.Corelib should this be something else? (Not for this PR).

src/libraries/Microsoft.Extensions.Configuration.UserSecrets/tests/MsBuildTargetTest.cs Show resolved Hide resolved
src/libraries/Microsoft.Extensions.Hosting/tests/FunctionalTests/ShutdownTests.cs
builder.AddXunit(_output);
});
// TODO refactor deployers to not depend on source code
// see https://github.com/dotnet/extensions/issues/1697 and https://github.com/dotnet/aspnetcore/issues/10268
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to either do this or disable the test. Since our tests run on helix machines without a clone of source this won't work.

src/libraries/Microsoft.Extensions.Http/tests/Microsoft.Extensions.Http.Tests.csproj Outdated Show resolved Hide resolved
...Microsoft.Extensions.Logging.EventSource/ref/Microsoft.Extensions.Logging.EventSource.csproj Show resolved Hide resolved
...Microsoft.Extensions.Logging.EventSource/src/Microsoft.Extensions.Logging.EventSource.csproj Outdated Show resolved Hide resolved
@maryamariyan maryamariyan mentioned this pull request Mar 24, 2020
Closed
1 task
@maryamariyan
Copy link
Member Author

Issued #34023 for the mono failure.
The failure hit on coreclr is already reported in #33930

This was referenced Mar 24, 2020
Closed
Closed
@maryamariyan maryamariyan force-pushed the all-ref-src branch 2 times, most recently from f2d1a62 to 19f8387 Compare March 25, 2020 05:10
@maryamariyan maryamariyan marked this pull request as ready for review March 25, 2020 05:20
@maryamariyan
Copy link
Member Author

maryamariyan commented Mar 25, 2020 •
edited
Loading

I updated the PR to enable FunctionalTests and TestApp. That required me to bring over history on the following folder in Extensions repo:
src/Hosting/IntegrationTesting/
I moved files there into
src/libraries/Microsoft.Extensions.Hosting/tests/FunctionalTests/IntegrationTesting/src/ 

Then to simplify the PR for review I squashed all changes into two commits on top of the merge history commit:
229db37
(which was mostly already reviewed)
and
19f8387
(which concludes FunctionalTests and TestApp on Hosting)

the commit messages hopefully make it easier to follow the changes

Also note, I manually brought over changes seen in the following PR so that I could remove Serilog dependency and complete enabling all tests:
dotnet/extensions#3040
(content of manual change can be found in 229db37)

@maryamariyan maryamariyan mentioned this pull request Mar 25, 2020
Merged
@maryamariyan
Configuration (ref/src/pkg/test)
099bec8 
Microsoft.Extensions.Configuration.Json
Microsoft.Extensions.Configurations.UserSecrets
Microsoft.Extensions.Configurations.Xml

Logging (ref/src/pkg/test)
- Microsoft.Extensions.Logging
- Microsoft.Extensions.Logging.Configuration
- Microsoft.Extensions.Logging.Console
- Microsoft.Extensions.Logging.Debug
- Microsoft.Extensions.Logging.EventLog
- Microsoft.Extensions.Logging.EventSource
- Microsoft.Extensions.Logging.TraceSource

ref\src\pkg\test
Microsoft.Extensions.Http
Microsoft.Extensions.Caching.Memory

Microsoft.Extensions.Hosting
- [x] Functional/TestApp
- Include Configuration.FunctionalTests and TestApp but refactor later

More:
- changes similar to 3040 in extensions
- skip some test failures using ActiveIssue
- ProjectExclusions removed
- Suppress the duplication LoggingBuilderExtensions
- deleting all csproj under Common
@maryamariyan
Make Hosting FunctionalTests and UnitTests work.
6c807c7 
- no more project exclusions
- delete files not used under Hosting/IntegrationTesting
@maryamariyan
Fix error error CA2012:
97a186d 
- ValueTask instances should not have their result directly accessed unless the instance has already completed.
@maryamariyan
- using OutputType Exe
b438b86 
- Logging.Tests needs Logging./Abstraction/Console/EventLog
@maryamariyan
Fix test build compile error
8b67dc5
@maryamariyan
add NetCoreAppCurrent to TargetFrameworks on TestApp
914fc70
@maryamariyan maryamariyan mentioned this pull request Mar 25, 2020
Open
@maryamariyan
Copy link
Member Author

maryamariyan commented Mar 25, 2020 •
edited
Loading

disabled failing tests with ActiveIssue linking to the corresponding open issues: #34090 and #34091

Left to fix: allConfiguration failure on transport package:
https://helix.dot.net/api/2019-06-17/jobs/aae824f2-25cc-411d-9b72-c253ad4d2f35/workitems/Microsoft.Extensions.Internal.Transport/console

@jaredpar jaredpar mentioned this pull request Mar 25, 2020
Closed
@maryamariyan maryamariyan merged commit db23750 into dotnet:master Mar 26, 2020
@jaredpar jaredpar mentioned this pull request Mar 26, 2020
Closed
@KalleOlaviNiemitalo
Copy link

It looks like Microsoft.Extensions.Logging.Testing.LogLevelAttribute no longer has any effect, although one AssemblyInfo.cs file still instantiates it. This PR deleted Microsoft.Extensions.Logging.Testing.LoggedTestBase, which was the only class that searched for LogLevelAttribute.

@ericstj
Copy link
Member

ericstj commented Sep 22, 2020

@KalleOlaviNiemitalo if it was your intent to report a problem, please open an issue with repro steps rather than comment on a closed pull request. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@safern safern safern left review comments

@ericstj ericstj ericstj approved these changes

Assignees
No one assigned
Labels
area-Meta
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@maryamariyan @KalleOlaviNiemitalo @ericstj @safern @Dotnet-GitSync-Bot