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

Move tests from full facade assemblies into type forward destination #89230

Closed
26 tasks done
ViktorHofer opened this issue Jul 20, 2023 · 7 comments · Fixed by #94644
Closed
26 tasks done

Move tests from full facade assemblies into type forward destination #89230

ViktorHofer opened this issue Jul 20, 2023 · 7 comments · Fixed by #94644
Assignees
@ViktorHofer
Labels
area-System.Runtime help wanted [up-for-grabs] Good issue for external contributors
Milestone
Future

Comments

@ViktorHofer
Copy link
Member

ViktorHofer commented Jul 20, 2023 •
edited
Loading

#89184 moved full façade assemblies into src/libraries/shims but left the tests in their original location. This issue tracks moving the tests and their test projects into the type forward destination (in most cases System.Runtime/tests/):

  • System.AppContext
  • System.Buffers
  • System.Data.DataSetExtensions
  • System.Diagnostics.Debug
  • System.Diagnostics.Tools
  • System.Dynamic.Runtime
  • System.Globalization
  • System.Globalization.Calendars
  • System.Globalization.Extensions
  • System.IO
  • System.IO.FileSystem
  • System.IO.FileSystem.Primitives
  • System.IO.UnmanagedMemoryStream
  • System.Reflection
  • System.Resources.Reader
  • System.Resources.ResourceManager
  • System.Runtime.CompilerServices.Unsafe
  • System.Runtime.Extensions
  • System.Runtime.Handles
  • System.Runtime.InteropServices.RuntimeInformation
  • System.Security.SecureString
  • System.Text.Encoding
  • System.Threading.Tasks
  • System.Threading.Tasks.Extensions
  • System.Threading.Timer
  • System.ValueTuple
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 20, 2023
@ghost
Copy link

ghost commented Jul 20, 2023

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

#89184 moved full façade assemblies into src/libraries/shims but left the tests in their original location. This issue tracks moving the tests into the type forward destination (in most cases System.Runtime):

  • System.AppContext
  • System.Buffers
  • System.Data.DataSetExtensions
  • System.Diagnostics.Debug
  • System.Diagnostics.Tools
  • System.Dynamic.Runtime
  • System.Globalization
  • System.Globalization.Calendars
  • System.Globalization.Extensions
  • System.IO
  • System.IO.FileSystem
  • System.IO.FileSystem.Primitives
  • System.IO.UnmanagedMemoryStream
  • System.Reflection
  • System.Resources.Reader
  • System.Resources.ResourceManager
  • System.Runtime.CompilerServices.Unsafe
  • System.Runtime.Extensions
  • System.Runtime.Handles
  • System.Runtime.InteropServices.RuntimeInformation
  • System.Security.SecureString
  • System.Text.Encoding
  • System.Threading.Tasks
  • System.Threading.Tasks.Extensions
  • System.Threading.Timer
  • System.ValueTuple
Author: ViktorHofer
Assignees: -
Labels:

area-Meta
Milestone: -

@ViktorHofer ViktorHofer mentioned this issue Jul 20, 2023
Merged
@ericstj
Copy link
Member

ericstj commented Jul 20, 2023

Is there an easy version of this that just moves the projects into subdirectories in System.Runtime\tests but doesn't do any project refactoring?

@ghost
Copy link

ghost commented Jul 20, 2023

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

#89184 moved full façade assemblies into src/libraries/shims but left the tests in their original location. This issue tracks moving the tests into the type forward destination (in most cases System.Runtime):

  • System.AppContext
  • System.Buffers
  • System.Data.DataSetExtensions
  • System.Diagnostics.Debug
  • System.Diagnostics.Tools
  • System.Dynamic.Runtime
  • System.Globalization
  • System.Globalization.Calendars
  • System.Globalization.Extensions
  • System.IO
  • System.IO.FileSystem
  • System.IO.FileSystem.Primitives
  • System.IO.UnmanagedMemoryStream
  • System.Reflection
  • System.Resources.Reader
  • System.Resources.ResourceManager
  • System.Runtime.CompilerServices.Unsafe
  • System.Runtime.Extensions
  • System.Runtime.Handles
  • System.Runtime.InteropServices.RuntimeInformation
  • System.Security.SecureString
  • System.Text.Encoding
  • System.Threading.Tasks
  • System.Threading.Tasks.Extensions
  • System.Threading.Timer
  • System.ValueTuple
Author: ViktorHofer
Assignees: -
Labels:

area-System.Runtime, untriaged
Milestone: -

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Jul 20, 2023 •
edited
Loading

What do you mean by project refactoring? I would hope that moving the test sources doesn't need any major msbuild work. As an example, moving System.AppContext.Tests.csproj doesn't require changing the project file: https://github.com/dotnet/runtime/blob/main/src/libraries/System.AppContext/tests/System.AppContext.Tests.csproj

@ericstj
Copy link
Member

ericstj commented Jul 21, 2023

I meant we could simply move the project files as is - giving a test folder with the same name as the test project - rather than trying to combine the sources with the existing System.Runtime.Tests.csproj.

@MichalStrehovsky
Copy link
Member

WASM is running into some implementation limitations in System.Text.Json test because they're too huge (#87078). If most of these are going to and up in System.Runtime.Tests, we might run into same impl limitations (I'm not clear what they are - just pointing to an issue that has them, with people that might understand more).

@ViktorHofer
Copy link
Member Author

Oh I see. I should have clarified in the top post. I think it would be best to move the tests as is, including their project files and not attempt to merge the test sources into the existing test projects.

Libraries' tests.proj disables test projects based on configurations (mobile vs NativeAOT vs wasm, ...). If one of these test projects is already disabled and we would now merge the test source code into an existing test project, those tests would need to be disabled inside the assembly.

@tannergooding tannergooding removed the untriaged New issue has not been triaged by the area owner label Jul 21, 2023
@tannergooding tannergooding added this to the Future milestone Jul 21, 2023
@ViktorHofer ViktorHofer added the help wanted [up-for-grabs] Good issue for external contributors label Oct 31, 2023
@ViktorHofer ViktorHofer self-assigned this Nov 12, 2023
@ViktorHofer ViktorHofer mentioned this issue Nov 12, 2023
Merged
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 12, 2023
ViktorHofer added a commit that referenced this issue Nov 12, 2023
@ViktorHofer
Move tests from full facade folder to type forward destination
cef8670 
Fixes #89230

A previous changed moved full facade assemblies into src/libraries/shims
but the tests were left in their original location.

1. Move System.Data.DataSetExtensions/tests into System.Data.Common/tests
and all other test projects under System.Runtime/tests.

2. Update the tests.proj file that excludes based on the test project file
paths.

3. Delete System.AppContext/tests which already existed (as a copy)
   under System.Runtime/tests.

4. Update System.Runtime.sln and System.Data.Common.sln

5. Delete README.md files that are now unused and not required anymore.
ViktorHofer added a commit that referenced this issue Nov 13, 2023
@ViktorHofer
Move tests from full facade folder to type forward destination
e80bc72 
Fixes #89230

A previous changed moved full facade assemblies into src/libraries/shims
but the tests were left in their original location.

1. Move System.Data.DataSetExtensions/tests into System.Data.Common/tests
and all other test projects under System.Runtime/tests.

2. Update the tests.proj file that excludes based on the test project file
paths.

3. Delete System.AppContext/tests which already existed (as a copy)
   under System.Runtime/tests.

4. Update System.Runtime.sln and System.Data.Common.sln

5. Delete README.md files that are now unused and not required anymore.
ViktorHofer added a commit that referenced this issue Nov 13, 2023
@ViktorHofer
Move tests from full facade folder to type forward destination (#94644)
71a7d88 
Fixes #89230

A previous changed moved full facade assemblies into src/libraries/shims
but the tests were left in their original location.

1. Move System.Data.DataSetExtensions/tests into System.Data.Common/tests
and all other test projects under System.Runtime/tests.

2. Update the tests.proj file that excludes based on the test project file
paths.

3. Delete System.AppContext/tests which already existed (as a copy)
   under System.Runtime/tests.

4. Update System.Runtime.sln and System.Data.Common.sln

5. Delete README.md files that are now unused and not required anymore.
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 13, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@ViktorHofer ViktorHofer

Labels
area-System.Runtime help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Milestone
Future
Development

Successfully merging a pull request may close this issue.

Move tests from full facade folder to type forward destination dotnet/runtime
4 participants
@ViktorHofer @ericstj @tannergooding @MichalStrehovsky