Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Add test for dotnet/coreclr#18521 fix #32538

Merged
merged 7 commits into from
Oct 24, 2018

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented Sep 28, 2018

Adds a unit test for the fix in dotnet/coreclr#20194 based on the original repro.

Adds a unit test for dotnet/coreclr#20263

@stephentoub
Copy link
Member

This is passing ci even though that coreclr PR isn't merged. Is that expected?

@jkoritzinsky
Copy link
Member Author

That is not expected. I must have messed up porting over the test from CoreCLR. I'll figure it out on Monday.

@jkoritzinsky jkoritzinsky added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 29, 2018
@jkoritzinsky jkoritzinsky force-pushed the fixes/coreclr/18521/tests branch from 598a62a to 143c138 Compare October 1, 2018 23:49
@jkoritzinsky jkoritzinsky removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 2, 2018
@jkoritzinsky jkoritzinsky changed the title Add test for dotnet/coreclr#18521 fix Add test for dotnet/coreclr#18521 fix and dotnet/coreclr#20263 Oct 5, 2018
@jkoritzinsky
Copy link
Member Author

As soon as CoreCLR commit 737d0ea is propagated to CoreFX this will be ready for review.

@jkotas
Copy link
Member

jkotas commented Oct 10, 2018

Note that this is a breaking change: It is changing non-throwing behavior into a throwing one. I do not think we should be making breaking changes like this.

@karelz
Copy link
Member

karelz commented Oct 12, 2018

@jkotas @luqunl @AaronRobinsonMSFT do you plan to revert the change in CoreCLR in that case?
Should these tests be just adjusted or do we need to discard them entirely?

@jkoritzinsky
Copy link
Member Author

We have another change in flight that modifies where the exception is thrown. I'll update the tests in this PR once we decide whether or not to merge that PR. (Some of the tests in this PR should be merged anyway since they weren't breaking changes)

@karelz
Copy link
Member

karelz commented Oct 12, 2018

Can you link the CoreCLR change in flight please?

@jkoritzinsky
Copy link
Member Author

dotnet/coreclr#20375

@jkoritzinsky jkoritzinsky force-pushed the fixes/coreclr/18521/tests branch from c9be05c to 5945e7b Compare October 24, 2018 18:25
@jkoritzinsky jkoritzinsky changed the title Add test for dotnet/coreclr#18521 fix and dotnet/coreclr#20263 Add test for dotnet/coreclr#18521 fix Oct 24, 2018
@jkoritzinsky
Copy link
Member Author

We've decided to take a different direction on dotnet/coreclr#20375 so I've removed that test from this PR. When CI is green this PR is ready to merge.

@jkoritzinsky jkoritzinsky merged commit ab753be into dotnet:master Oct 24, 2018
@jkoritzinsky jkoritzinsky deleted the fixes/coreclr/18521/tests branch October 24, 2018 22:56
@karelz karelz added this to the 3.0 milestone Nov 15, 2018
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* Add test for dotnet/coreclrdotnet/corefx#18521

* Update StructureToPtrTests.cs

* Add additional test case.

* Disable tests on .NET Framework.

* Add opaque struct in non-blittable struct test.


Commit migrated from dotnet/corefx@ab753be
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants