Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

WIP Update base services tests to SDK style projects#19282

Closed
chsienki wants to merge 1 commit into
dotnet:masterfrom
chsienki:upgrade_test_projects_sdk
Closed

WIP Update base services tests to SDK style projects#19282
chsienki wants to merge 1 commit into
dotnet:masterfrom
chsienki:upgrade_test_projects_sdk

Conversation

@chsienki
Copy link
Copy Markdown
Member

@chsienki chsienki commented Aug 3, 2018

Update all the tests in baseservices to be SDK style projects

@chsienki chsienki added * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) 2 - In Progress labels Aug 3, 2018
@chsienki chsienki force-pushed the upgrade_test_projects_sdk branch from f61ef13 to c80ef42 Compare August 3, 2018 22:58
@chsienki
Copy link
Copy Markdown
Member Author

chsienki commented Aug 3, 2018

@dotnet-bot test Windows_NT x86 Checked Build and Test
@dotnet-bot test Windows_NT x64 Build and Test

</ItemGroup>
<ItemGroup>
<ProjectReference Include="StringThrower.ilproj" />
<NoWarn Include="42016,42020,42025,42024" />
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That's wrong semantics as all upper level NoWarn settings are lost - should be

<NoWarn Include="42016,42020,42025,42024,$(NoWarn)" />

I know that it is old code but perhaps you could fix it with some freecycles, or not 😄

<Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
</ItemGroup>
<ItemGroup>
<ProjectReference Include="../../../Common/CoreCLRTestLibrary/CoreCLRTestLibrary.csproj" />
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All absolute references to CoreCLRTestLibrary.csproj were causing a lot of problems - mainly warnings. I have fixed to them to relative paths rooted in tests\src\ directory - pls see #19275 commit d265f21

<Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
</ItemGroup>
<ItemGroup>
<ProjectReference Include="../../../Common/CoreCLRTestLibrary/CoreCLRTestLibrary.csproj" />
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

same as above

@4creators
Copy link
Copy Markdown

@chsienki Pls take a look at #19275 as the changes are partially overlapping and conflicting

@ViktorHofer
Copy link
Copy Markdown
Member

Hey @chsienki, what's the status on this? I would really love to see this going into master.

</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'Release|x64'">
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'Debug|x64'"></PropertyGroup>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

with the project-sdk csproj format you don't need these anymore. Instead we should use the Configurations property, i.e. https://github.com/dotnet/corefx/blob/master/src/System.Text.RegularExpressions/src/System.Text.RegularExpressions.csproj#L9

@jkotas
Copy link
Copy Markdown
Member

jkotas commented May 24, 2019

Closing stale PRs.

@jkotas jkotas closed this May 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

* NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants