Roll back conversion of native aot tests to merged model by MichalStrehovsky · Pull Request #100269 · dotnet/runtime

MichalStrehovsky · 2024-03-26T06:55:04Z

There are two issues with this:

1. The merged tests are actually broken, but the failures are only observable without Helix

Helix does not report the failures (this is the reason I’m doing a rollback right away instead of waiting around for a fix). Example:

runtime/src/tests/nativeaot/SmokeTests/Exceptions/Exceptions.cs

Lines 67 to 71 in 78a895f

if(!stackTrace.Contains("BringUpTest.Main"))
{
Console.WriteLine("Unexpected stack trace: "+stackTrace);
returnFail;
}

The above should obviously fail because there is no BringUpTest.Main (it got renamed to BringUpTest.TestEntryPoint as part of merging). The CI is all green, despite of this. The test fails if I run it locally through:

build clr.aot+libs -rc Checked -lc Release
src\tests\build.cmd nativeaot checked tree nativeaot\SmokeTests
src\tests\run.cmd runnativeaottests checked

2. There are several test failures, some of which look like they’ll need fixes in the merged wrapper infrastructure

The Exceptions test wants to be able to test AppDomain.UnhandledException:
runtime/src/tests/nativeaot/SmokeTests/Exceptions/Exceptions.cs
Line 43 in 78a895f
AppDomain.CurrentDomain.UnhandledException+=UnhandledExceptionEventHandler;

The wrapper however generates a Try/Catch that doesn’t let UnhandledException happen. The test fails.
The TrimmingBehaviors test is sensitive to ripple effects in whole program view. The wrapper’s use of Xunit Assert.Equal brings in a massive amount of reflection that causes ripple effects all over the place. The test fails in a rather obscure way and it took a while to root cause.

I’m also concerned about whether we have any tests that may now be succeeding because of the ripple effects from the wrapper (and we would not notice if the thing they are testing gets broken because they get saved by the ripple effects.

To fix this one, can we make it so RequiresProcessIsolation really just generates a Main that calls the other Main, without checking exit codes, wrapping anything, Assert.Equaling etc?

Cc @dotnet/ilc-contrib

This reverts commit 43b55fa.

dotnet#98469)" This reverts commit 41c6057.

dotnet-policy-service · 2024-03-26T06:55:45Z

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

jkotas · 2024-03-26T11:19:22Z

Helix does not report the failures

Does this affect all merged tests or just the nativeaot ones that you are reverting?

MichalStrehovsky · 2024-03-26T11:31:07Z

Helix does not report the failures
Does this affect all merged tests or just the nativeaot ones that you are reverting?

I'm afraid of the answer to this. I didn't check. Smoke tests are absolutely critical.

jkotas · 2024-03-27T00:29:41Z

I'm afraid of the answer to this. I didn't check.

Checking: #100319

agocke · 2024-03-27T01:24:01Z

The wrapper’s use of Xunit Assert.Equal brings in a massive amount of reflection that causes ripple effects all over the place.

This shouldn't be true anymore. The Microsoft.Dotnet.XUnitAssert package should sub in and it should be trim safe.

jkotas · 2024-03-27T03:54:50Z

I'm afraid of the answer to this. I didn't check.

The other runtime tests are failing as expected https://dev.azure.com/dnceng-public/public/_build/results?buildId=620112&view=ms.vss-test-web.build-test-results-tab .

MichalStrehovsky · 2024-03-27T05:21:01Z

The wrapper’s use of Xunit Assert.Equal brings in a massive amount of reflection that causes ripple effects all over the place.
This shouldn't be true anymore. The Microsoft.Dotnet.XUnitAssert package should sub in and it should be trim safe.

Trim safety is not the problem here, it's the use of reflection on random stuff in the BCL.

In the case I saw, somewhere after Assert.Equal<T> we marked methods on IEquatable<T> as reflected on and that was breaking what a test was trying to exercise because we were no longer able to remove something. (It took extra long to troubleshoot because I thought it's something I broke, only to find out the test is broken in main.)

The tests under nativeaot need to have very precise control over what's in the dependency graph to be able to test the things they are testing. The implementation of Assert.Equal that still depends on reflection doesn't feel like what we'd want in the runtime test tree (it's fine for libs testing if it's trim safe, but runtime tests should have as little code running as part of the test infrastructure as possible).

MichalStrehovsky · 2024-03-27T05:23:47Z

I'm afraid of the answer to this. I didn't check.
The other runtime tests are failing as expected https://dev.azure.com/dnceng-public/public/_build/results?buildId=620112&view=ms.vss-test-web.build-test-results-tab .

Thanks for re-running the check - I also suspect this might be related to RequiresProcessIsolation; there's not much else that is different in these tests. We'll see if that's the case soon enough...

MichalStrehovsky · 2024-03-27T08:55:53Z

Can anyone sign off? It's not acceptable to have main in a state where we basically don't run nativeaot testing for a week.

This needs to be merged before #100331 can fix testing RequiresProcessIsolation tests.

The test failure is an unrelated GC issue.

MichalStrehovsky · 2024-03-27T11:32:16Z

Thanks!

This is unfortunate because it was added for native AOT sake in dotnet#91415. Then Tomas rewrote the test to execute corerun.exe in dotnet#91560 making it 100% unsupportable with native AOT. What the test is doing doesn't look supportable with the merged runner infra and runs into the same issues I saw in dotnet#100269. We never noticed this test is broken because tests requiring process isolation are completely busted with native AOT. I did a local run and started re-baselining again because this is not the only regression.

This is unfortunate because it was added for native AOT sake in #91415. Then Tomas rewrote the test to execute corerun.exe in #91560 making it 100% unsupportable with native AOT. What the test is doing doesn't look supportable with the merged runner infra and runs into the same issues I saw in #100269. We never noticed this test is broken because tests requiring process isolation are completely busted with native AOT. I did a local run and started re-baselining again because this is not the only regression.

MichalStrehovsky · 2024-03-26T06:55:04Z

There are two issues with this:

1. The merged tests are actually broken, but the failures are only observable without Helix

Helix does not report the failures (this is the reason I’m doing a rollback right away instead of waiting around for a fix). Example:

runtime/src/tests/nativeaot/SmokeTests/Exceptions/Exceptions.cs

Lines 67 to 71 in 78a895f

if(!stackTrace.Contains("BringUpTest.Main"))
{
Console.WriteLine("Unexpected stack trace: "+stackTrace);
returnFail;
}

The above should obviously fail because there is no BringUpTest.Main (it got renamed to BringUpTest.TestEntryPoint as part of merging). The CI is all green, despite of this. The test fails if I run it locally through:

build clr.aot+libs -rc Checked -lc Release
src\tests\build.cmd nativeaot checked tree nativeaot\SmokeTests
src\tests\run.cmd runnativeaottests checked

2. There are several test failures, some of which look like they’ll need fixes in the merged wrapper infrastructure

The Exceptions test wants to be able to test AppDomain.UnhandledException:
runtime/src/tests/nativeaot/SmokeTests/Exceptions/Exceptions.cs
Line 43 in 78a895f
AppDomain.CurrentDomain.UnhandledException+=UnhandledExceptionEventHandler;

The wrapper however generates a Try/Catch that doesn’t let UnhandledException happen. The test fails.
The TrimmingBehaviors test is sensitive to ripple effects in whole program view. The wrapper’s use of Xunit Assert.Equal brings in a massive amount of reflection that causes ripple effects all over the place. The test fails in a rather obscure way and it took a while to root cause.

I’m also concerned about whether we have any tests that may now be succeeding because of the ripple effects from the wrapper (and we would not notice if the thing they are testing gets broken because they get saved by the ripple effects.

To fix this one, can we make it so RequiresProcessIsolation really just generates a Main that calls the other Main, without checking exit codes, wrapping anything, Assert.Equaling etc?

Cc @dotnet/ilc-contrib

This reverts commit 43b55fa.

dotnet#98469)" This reverts commit 41c6057.

dotnet-policy-service · 2024-03-26T06:55:45Z

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

jkotas · 2024-03-26T11:19:22Z

Helix does not report the failures

Does this affect all merged tests or just the nativeaot ones that you are reverting?

MichalStrehovsky · 2024-03-26T11:31:07Z

Helix does not report the failures
Does this affect all merged tests or just the nativeaot ones that you are reverting?

I'm afraid of the answer to this. I didn't check. Smoke tests are absolutely critical.

jkotas · 2024-03-27T00:29:41Z

I'm afraid of the answer to this. I didn't check.

Checking: #100319

agocke · 2024-03-27T01:24:01Z

The wrapper’s use of Xunit Assert.Equal brings in a massive amount of reflection that causes ripple effects all over the place.

This shouldn't be true anymore. The Microsoft.Dotnet.XUnitAssert package should sub in and it should be trim safe.

jkotas · 2024-03-27T03:54:50Z

I'm afraid of the answer to this. I didn't check.

The other runtime tests are failing as expected https://dev.azure.com/dnceng-public/public/_build/results?buildId=620112&view=ms.vss-test-web.build-test-results-tab .

MichalStrehovsky · 2024-03-27T05:21:01Z

The wrapper’s use of Xunit Assert.Equal brings in a massive amount of reflection that causes ripple effects all over the place.
This shouldn't be true anymore. The Microsoft.Dotnet.XUnitAssert package should sub in and it should be trim safe.

Trim safety is not the problem here, it's the use of reflection on random stuff in the BCL.

In the case I saw, somewhere after Assert.Equal<T> we marked methods on IEquatable<T> as reflected on and that was breaking what a test was trying to exercise because we were no longer able to remove something. (It took extra long to troubleshoot because I thought it's something I broke, only to find out the test is broken in main.)

The tests under nativeaot need to have very precise control over what's in the dependency graph to be able to test the things they are testing. The implementation of Assert.Equal that still depends on reflection doesn't feel like what we'd want in the runtime test tree (it's fine for libs testing if it's trim safe, but runtime tests should have as little code running as part of the test infrastructure as possible).

MichalStrehovsky · 2024-03-27T05:23:47Z

I'm afraid of the answer to this. I didn't check.
The other runtime tests are failing as expected https://dev.azure.com/dnceng-public/public/_build/results?buildId=620112&view=ms.vss-test-web.build-test-results-tab .

Thanks for re-running the check - I also suspect this might be related to RequiresProcessIsolation; there's not much else that is different in these tests. We'll see if that's the case soon enough...

MichalStrehovsky · 2024-03-27T08:55:53Z

Can anyone sign off? It's not acceptable to have main in a state where we basically don't run nativeaot testing for a week.

This needs to be merged before #100331 can fix testing RequiresProcessIsolation tests.

The test failure is an unrelated GC issue.

MichalStrehovsky · 2024-03-27T11:32:16Z

Thanks!

This is unfortunate because it was added for native AOT sake in dotnet#91415. Then Tomas rewrote the test to execute corerun.exe in dotnet#91560 making it 100% unsupportable with native AOT. What the test is doing doesn't look supportable with the merged runner infra and runs into the same issues I saw in dotnet#100269. We never noticed this test is broken because tests requiring process isolation are completely busted with native AOT. I did a local run and started re-baselining again because this is not the only regression.

This is unfortunate because it was added for native AOT sake in #91415. Then Tomas rewrote the test to execute corerun.exe in #91560 making it 100% unsupportable with native AOT. What the test is doing doesn't look supportable with the merged runner infra and runs into the same issues I saw in #100269. We never noticed this test is broken because tests requiring process isolation are completely busted with native AOT. I did a local run and started re-baselining again because this is not the only regression.

MichalStrehovsky · 2024-03-26T06:55:04Z

There are two issues with this:

1. The merged tests are actually broken, but the failures are only observable without Helix

Helix does not report the failures (this is the reason I’m doing a rollback right away instead of waiting around for a fix). Example:

runtime/src/tests/nativeaot/SmokeTests/Exceptions/Exceptions.cs

Lines 67 to 71 in 78a895f

if(!stackTrace.Contains("BringUpTest.Main"))
{
Console.WriteLine("Unexpected stack trace: "+stackTrace);
returnFail;
}

The above should obviously fail because there is no BringUpTest.Main (it got renamed to BringUpTest.TestEntryPoint as part of merging). The CI is all green, despite of this. The test fails if I run it locally through:

build clr.aot+libs -rc Checked -lc Release
src\tests\build.cmd nativeaot checked tree nativeaot\SmokeTests
src\tests\run.cmd runnativeaottests checked

2. There are several test failures, some of which look like they’ll need fixes in the merged wrapper infrastructure

The Exceptions test wants to be able to test AppDomain.UnhandledException:
runtime/src/tests/nativeaot/SmokeTests/Exceptions/Exceptions.cs
Line 43 in 78a895f
AppDomain.CurrentDomain.UnhandledException+=UnhandledExceptionEventHandler;

The wrapper however generates a Try/Catch that doesn’t let UnhandledException happen. The test fails.
The TrimmingBehaviors test is sensitive to ripple effects in whole program view. The wrapper’s use of Xunit Assert.Equal brings in a massive amount of reflection that causes ripple effects all over the place. The test fails in a rather obscure way and it took a while to root cause.

I’m also concerned about whether we have any tests that may now be succeeding because of the ripple effects from the wrapper (and we would not notice if the thing they are testing gets broken because they get saved by the ripple effects.

To fix this one, can we make it so RequiresProcessIsolation really just generates a Main that calls the other Main, without checking exit codes, wrapping anything, Assert.Equaling etc?

Cc @dotnet/ilc-contrib

This reverts commit 43b55fa.

dotnet#98469)" This reverts commit 41c6057.

dotnet-policy-service · 2024-03-26T06:55:45Z

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

jkotas · 2024-03-26T11:19:22Z

Helix does not report the failures

Does this affect all merged tests or just the nativeaot ones that you are reverting?

MichalStrehovsky · 2024-03-26T11:31:07Z

Helix does not report the failures
Does this affect all merged tests or just the nativeaot ones that you are reverting?

I'm afraid of the answer to this. I didn't check. Smoke tests are absolutely critical.

jkotas · 2024-03-27T00:29:41Z

I'm afraid of the answer to this. I didn't check.

Checking: #100319

agocke · 2024-03-27T01:24:01Z

The wrapper’s use of Xunit Assert.Equal brings in a massive amount of reflection that causes ripple effects all over the place.

This shouldn't be true anymore. The Microsoft.Dotnet.XUnitAssert package should sub in and it should be trim safe.

jkotas · 2024-03-27T03:54:50Z

I'm afraid of the answer to this. I didn't check.

The other runtime tests are failing as expected https://dev.azure.com/dnceng-public/public/_build/results?buildId=620112&view=ms.vss-test-web.build-test-results-tab .

MichalStrehovsky · 2024-03-27T05:21:01Z

The wrapper’s use of Xunit Assert.Equal brings in a massive amount of reflection that causes ripple effects all over the place.
This shouldn't be true anymore. The Microsoft.Dotnet.XUnitAssert package should sub in and it should be trim safe.

Trim safety is not the problem here, it's the use of reflection on random stuff in the BCL.

In the case I saw, somewhere after Assert.Equal<T> we marked methods on IEquatable<T> as reflected on and that was breaking what a test was trying to exercise because we were no longer able to remove something. (It took extra long to troubleshoot because I thought it's something I broke, only to find out the test is broken in main.)

The tests under nativeaot need to have very precise control over what's in the dependency graph to be able to test the things they are testing. The implementation of Assert.Equal that still depends on reflection doesn't feel like what we'd want in the runtime test tree (it's fine for libs testing if it's trim safe, but runtime tests should have as little code running as part of the test infrastructure as possible).

MichalStrehovsky · 2024-03-27T05:23:47Z

I'm afraid of the answer to this. I didn't check.
The other runtime tests are failing as expected https://dev.azure.com/dnceng-public/public/_build/results?buildId=620112&view=ms.vss-test-web.build-test-results-tab .

Thanks for re-running the check - I also suspect this might be related to RequiresProcessIsolation; there's not much else that is different in these tests. We'll see if that's the case soon enough...

MichalStrehovsky · 2024-03-27T08:55:53Z

Can anyone sign off? It's not acceptable to have main in a state where we basically don't run nativeaot testing for a week.

This needs to be merged before #100331 can fix testing RequiresProcessIsolation tests.

The test failure is an unrelated GC issue.

MichalStrehovsky · 2024-03-27T11:32:16Z

Thanks!

This is unfortunate because it was added for native AOT sake in dotnet#91415. Then Tomas rewrote the test to execute corerun.exe in dotnet#91560 making it 100% unsupportable with native AOT. What the test is doing doesn't look supportable with the merged runner infra and runs into the same issues I saw in dotnet#100269. We never noticed this test is broken because tests requiring process isolation are completely busted with native AOT. I did a local run and started re-baselining again because this is not the only regression.

This is unfortunate because it was added for native AOT sake in #91415. Then Tomas rewrote the test to execute corerun.exe in #91560 making it 100% unsupportable with native AOT. What the test is doing doesn't look supportable with the merged runner infra and runs into the same issues I saw in #100269. We never noticed this test is broken because tests requiring process isolation are completely busted with native AOT. I did a local run and started re-baselining again because this is not the only regression.

MichalStrehovsky · 2024-03-26T06:55:04Z

There are two issues with this:

1. The merged tests are actually broken, but the failures are only observable without Helix

Helix does not report the failures (this is the reason I’m doing a rollback right away instead of waiting around for a fix). Example:

runtime/src/tests/nativeaot/SmokeTests/Exceptions/Exceptions.cs

Lines 67 to 71 in 78a895f

if(!stackTrace.Contains("BringUpTest.Main"))
{
Console.WriteLine("Unexpected stack trace: "+stackTrace);
returnFail;
}

The above should obviously fail because there is no BringUpTest.Main (it got renamed to BringUpTest.TestEntryPoint as part of merging). The CI is all green, despite of this. The test fails if I run it locally through:

build clr.aot+libs -rc Checked -lc Release
src\tests\build.cmd nativeaot checked tree nativeaot\SmokeTests
src\tests\run.cmd runnativeaottests checked

2. There are several test failures, some of which look like they’ll need fixes in the merged wrapper infrastructure

The Exceptions test wants to be able to test AppDomain.UnhandledException:
runtime/src/tests/nativeaot/SmokeTests/Exceptions/Exceptions.cs
Line 43 in 78a895f
AppDomain.CurrentDomain.UnhandledException+=UnhandledExceptionEventHandler;

The wrapper however generates a Try/Catch that doesn’t let UnhandledException happen. The test fails.
The TrimmingBehaviors test is sensitive to ripple effects in whole program view. The wrapper’s use of Xunit Assert.Equal brings in a massive amount of reflection that causes ripple effects all over the place. The test fails in a rather obscure way and it took a while to root cause.

I’m also concerned about whether we have any tests that may now be succeeding because of the ripple effects from the wrapper (and we would not notice if the thing they are testing gets broken because they get saved by the ripple effects.

To fix this one, can we make it so RequiresProcessIsolation really just generates a Main that calls the other Main, without checking exit codes, wrapping anything, Assert.Equaling etc?

Cc @dotnet/ilc-contrib

This reverts commit 43b55fa.

dotnet#98469)" This reverts commit 41c6057.

dotnet-policy-service · 2024-03-26T06:55:45Z

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

jkotas · 2024-03-26T11:19:22Z

Helix does not report the failures

Does this affect all merged tests or just the nativeaot ones that you are reverting?

MichalStrehovsky · 2024-03-26T11:31:07Z

Helix does not report the failures
Does this affect all merged tests or just the nativeaot ones that you are reverting?

I'm afraid of the answer to this. I didn't check. Smoke tests are absolutely critical.

jkotas · 2024-03-27T00:29:41Z

I'm afraid of the answer to this. I didn't check.

Checking: #100319

agocke · 2024-03-27T01:24:01Z

The wrapper’s use of Xunit Assert.Equal brings in a massive amount of reflection that causes ripple effects all over the place.

This shouldn't be true anymore. The Microsoft.Dotnet.XUnitAssert package should sub in and it should be trim safe.

jkotas · 2024-03-27T03:54:50Z

I'm afraid of the answer to this. I didn't check.

The other runtime tests are failing as expected https://dev.azure.com/dnceng-public/public/_build/results?buildId=620112&view=ms.vss-test-web.build-test-results-tab .

MichalStrehovsky · 2024-03-27T05:21:01Z

The wrapper’s use of Xunit Assert.Equal brings in a massive amount of reflection that causes ripple effects all over the place.
This shouldn't be true anymore. The Microsoft.Dotnet.XUnitAssert package should sub in and it should be trim safe.

Trim safety is not the problem here, it's the use of reflection on random stuff in the BCL.

In the case I saw, somewhere after Assert.Equal<T> we marked methods on IEquatable<T> as reflected on and that was breaking what a test was trying to exercise because we were no longer able to remove something. (It took extra long to troubleshoot because I thought it's something I broke, only to find out the test is broken in main.)

The tests under nativeaot need to have very precise control over what's in the dependency graph to be able to test the things they are testing. The implementation of Assert.Equal that still depends on reflection doesn't feel like what we'd want in the runtime test tree (it's fine for libs testing if it's trim safe, but runtime tests should have as little code running as part of the test infrastructure as possible).

MichalStrehovsky · 2024-03-27T05:23:47Z

I'm afraid of the answer to this. I didn't check.
The other runtime tests are failing as expected https://dev.azure.com/dnceng-public/public/_build/results?buildId=620112&view=ms.vss-test-web.build-test-results-tab .

Thanks for re-running the check - I also suspect this might be related to RequiresProcessIsolation; there's not much else that is different in these tests. We'll see if that's the case soon enough...

MichalStrehovsky · 2024-03-27T08:55:53Z

Can anyone sign off? It's not acceptable to have main in a state where we basically don't run nativeaot testing for a week.

This needs to be merged before #100331 can fix testing RequiresProcessIsolation tests.

The test failure is an unrelated GC issue.

MichalStrehovsky · 2024-03-27T11:32:16Z

Thanks!

This is unfortunate because it was added for native AOT sake in dotnet#91415. Then Tomas rewrote the test to execute corerun.exe in dotnet#91560 making it 100% unsupportable with native AOT. What the test is doing doesn't look supportable with the merged runner infra and runs into the same issues I saw in dotnet#100269. We never noticed this test is broken because tests requiring process isolation are completely busted with native AOT. I did a local run and started re-baselining again because this is not the only regression.

This is unfortunate because it was added for native AOT sake in #91415. Then Tomas rewrote the test to execute corerun.exe in #91560 making it 100% unsupportable with native AOT. What the test is doing doesn't look supportable with the merged runner infra and runs into the same issues I saw in #100269. We never noticed this test is broken because tests requiring process isolation are completely busted with native AOT. I did a local run and started re-baselining again because this is not the only regression.

MichalStrehovsky · 2024-03-26T06:55:04Z

There are two issues with this:

1. The merged tests are actually broken, but the failures are only observable without Helix

Helix does not report the failures (this is the reason I’m doing a rollback right away instead of waiting around for a fix). Example:

runtime/src/tests/nativeaot/SmokeTests/Exceptions/Exceptions.cs

Lines 67 to 71 in 78a895f

if(!stackTrace.Contains("BringUpTest.Main"))
{
Console.WriteLine("Unexpected stack trace: "+stackTrace);
returnFail;
}

The above should obviously fail because there is no BringUpTest.Main (it got renamed to BringUpTest.TestEntryPoint as part of merging). The CI is all green, despite of this. The test fails if I run it locally through:

build clr.aot+libs -rc Checked -lc Release
src\tests\build.cmd nativeaot checked tree nativeaot\SmokeTests
src\tests\run.cmd runnativeaottests checked

2. There are several test failures, some of which look like they’ll need fixes in the merged wrapper infrastructure

The Exceptions test wants to be able to test AppDomain.UnhandledException:
runtime/src/tests/nativeaot/SmokeTests/Exceptions/Exceptions.cs
Line 43 in 78a895f
AppDomain.CurrentDomain.UnhandledException+=UnhandledExceptionEventHandler;

The wrapper however generates a Try/Catch that doesn’t let UnhandledException happen. The test fails.
The TrimmingBehaviors test is sensitive to ripple effects in whole program view. The wrapper’s use of Xunit Assert.Equal brings in a massive amount of reflection that causes ripple effects all over the place. The test fails in a rather obscure way and it took a while to root cause.

I’m also concerned about whether we have any tests that may now be succeeding because of the ripple effects from the wrapper (and we would not notice if the thing they are testing gets broken because they get saved by the ripple effects.

To fix this one, can we make it so RequiresProcessIsolation really just generates a Main that calls the other Main, without checking exit codes, wrapping anything, Assert.Equaling etc?

Cc @dotnet/ilc-contrib

This reverts commit 43b55fa.

dotnet#98469)" This reverts commit 41c6057.

dotnet-policy-service · 2024-03-26T06:55:45Z

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

jkotas · 2024-03-26T11:19:22Z

Helix does not report the failures

Does this affect all merged tests or just the nativeaot ones that you are reverting?

MichalStrehovsky · 2024-03-26T11:31:07Z

Helix does not report the failures
Does this affect all merged tests or just the nativeaot ones that you are reverting?

I'm afraid of the answer to this. I didn't check. Smoke tests are absolutely critical.

jkotas · 2024-03-27T00:29:41Z

I'm afraid of the answer to this. I didn't check.

Checking: #100319

agocke · 2024-03-27T01:24:01Z

The wrapper’s use of Xunit Assert.Equal brings in a massive amount of reflection that causes ripple effects all over the place.

This shouldn't be true anymore. The Microsoft.Dotnet.XUnitAssert package should sub in and it should be trim safe.

jkotas · 2024-03-27T03:54:50Z

I'm afraid of the answer to this. I didn't check.

The other runtime tests are failing as expected https://dev.azure.com/dnceng-public/public/_build/results?buildId=620112&view=ms.vss-test-web.build-test-results-tab .

MichalStrehovsky · 2024-03-27T05:21:01Z

The wrapper’s use of Xunit Assert.Equal brings in a massive amount of reflection that causes ripple effects all over the place.
This shouldn't be true anymore. The Microsoft.Dotnet.XUnitAssert package should sub in and it should be trim safe.

Trim safety is not the problem here, it's the use of reflection on random stuff in the BCL.

In the case I saw, somewhere after Assert.Equal<T> we marked methods on IEquatable<T> as reflected on and that was breaking what a test was trying to exercise because we were no longer able to remove something. (It took extra long to troubleshoot because I thought it's something I broke, only to find out the test is broken in main.)

The tests under nativeaot need to have very precise control over what's in the dependency graph to be able to test the things they are testing. The implementation of Assert.Equal that still depends on reflection doesn't feel like what we'd want in the runtime test tree (it's fine for libs testing if it's trim safe, but runtime tests should have as little code running as part of the test infrastructure as possible).

MichalStrehovsky · 2024-03-27T05:23:47Z

I'm afraid of the answer to this. I didn't check.
The other runtime tests are failing as expected https://dev.azure.com/dnceng-public/public/_build/results?buildId=620112&view=ms.vss-test-web.build-test-results-tab .

Thanks for re-running the check - I also suspect this might be related to RequiresProcessIsolation; there's not much else that is different in these tests. We'll see if that's the case soon enough...

MichalStrehovsky · 2024-03-27T08:55:53Z

Can anyone sign off? It's not acceptable to have main in a state where we basically don't run nativeaot testing for a week.

This needs to be merged before #100331 can fix testing RequiresProcessIsolation tests.

The test failure is an unrelated GC issue.

MichalStrehovsky · 2024-03-27T11:32:16Z

Thanks!

This is unfortunate because it was added for native AOT sake in dotnet#91415. Then Tomas rewrote the test to execute corerun.exe in dotnet#91560 making it 100% unsupportable with native AOT. What the test is doing doesn't look supportable with the merged runner infra and runs into the same issues I saw in dotnet#100269. We never noticed this test is broken because tests requiring process isolation are completely busted with native AOT. I did a local run and started re-baselining again because this is not the only regression.

This is unfortunate because it was added for native AOT sake in #91415. Then Tomas rewrote the test to execute corerun.exe in #91560 making it 100% unsupportable with native AOT. What the test is doing doesn't look supportable with the merged runner infra and runs into the same issues I saw in #100269. We never noticed this test is broken because tests requiring process isolation are completely busted with native AOT. I did a local run and started re-baselining again because this is not the only regression.

MichalStrehovsky · 2024-03-26T06:55:04Z

There are two issues with this:

1. The merged tests are actually broken, but the failures are only observable without Helix

Helix does not report the failures (this is the reason I’m doing a rollback right away instead of waiting around for a fix). Example:

runtime/src/tests/nativeaot/SmokeTests/Exceptions/Exceptions.cs

Lines 67 to 71 in 78a895f

if(!stackTrace.Contains("BringUpTest.Main"))
{
Console.WriteLine("Unexpected stack trace: "+stackTrace);
returnFail;
}

The above should obviously fail because there is no BringUpTest.Main (it got renamed to BringUpTest.TestEntryPoint as part of merging). The CI is all green, despite of this. The test fails if I run it locally through:

build clr.aot+libs -rc Checked -lc Release
src\tests\build.cmd nativeaot checked tree nativeaot\SmokeTests
src\tests\run.cmd runnativeaottests checked

2. There are several test failures, some of which look like they’ll need fixes in the merged wrapper infrastructure

The Exceptions test wants to be able to test AppDomain.UnhandledException:
runtime/src/tests/nativeaot/SmokeTests/Exceptions/Exceptions.cs
Line 43 in 78a895f
AppDomain.CurrentDomain.UnhandledException+=UnhandledExceptionEventHandler;

The wrapper however generates a Try/Catch that doesn’t let UnhandledException happen. The test fails.
The TrimmingBehaviors test is sensitive to ripple effects in whole program view. The wrapper’s use of Xunit Assert.Equal brings in a massive amount of reflection that causes ripple effects all over the place. The test fails in a rather obscure way and it took a while to root cause.

I’m also concerned about whether we have any tests that may now be succeeding because of the ripple effects from the wrapper (and we would not notice if the thing they are testing gets broken because they get saved by the ripple effects.

To fix this one, can we make it so RequiresProcessIsolation really just generates a Main that calls the other Main, without checking exit codes, wrapping anything, Assert.Equaling etc?

Cc @dotnet/ilc-contrib

This reverts commit 43b55fa.

dotnet#98469)" This reverts commit 41c6057.

dotnet-policy-service · 2024-03-26T06:55:45Z

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

jkotas · 2024-03-26T11:19:22Z

Helix does not report the failures

Does this affect all merged tests or just the nativeaot ones that you are reverting?

MichalStrehovsky · 2024-03-26T11:31:07Z

Helix does not report the failures
Does this affect all merged tests or just the nativeaot ones that you are reverting?

I'm afraid of the answer to this. I didn't check. Smoke tests are absolutely critical.

jkotas · 2024-03-27T00:29:41Z

I'm afraid of the answer to this. I didn't check.

Checking: #100319

agocke · 2024-03-27T01:24:01Z

The wrapper’s use of Xunit Assert.Equal brings in a massive amount of reflection that causes ripple effects all over the place.

This shouldn't be true anymore. The Microsoft.Dotnet.XUnitAssert package should sub in and it should be trim safe.

jkotas · 2024-03-27T03:54:50Z

I'm afraid of the answer to this. I didn't check.

The other runtime tests are failing as expected https://dev.azure.com/dnceng-public/public/_build/results?buildId=620112&view=ms.vss-test-web.build-test-results-tab .

MichalStrehovsky · 2024-03-27T05:21:01Z

The wrapper’s use of Xunit Assert.Equal brings in a massive amount of reflection that causes ripple effects all over the place.
This shouldn't be true anymore. The Microsoft.Dotnet.XUnitAssert package should sub in and it should be trim safe.

Trim safety is not the problem here, it's the use of reflection on random stuff in the BCL.

In the case I saw, somewhere after Assert.Equal<T> we marked methods on IEquatable<T> as reflected on and that was breaking what a test was trying to exercise because we were no longer able to remove something. (It took extra long to troubleshoot because I thought it's something I broke, only to find out the test is broken in main.)

The tests under nativeaot need to have very precise control over what's in the dependency graph to be able to test the things they are testing. The implementation of Assert.Equal that still depends on reflection doesn't feel like what we'd want in the runtime test tree (it's fine for libs testing if it's trim safe, but runtime tests should have as little code running as part of the test infrastructure as possible).

MichalStrehovsky · 2024-03-27T05:23:47Z

I'm afraid of the answer to this. I didn't check.
The other runtime tests are failing as expected https://dev.azure.com/dnceng-public/public/_build/results?buildId=620112&view=ms.vss-test-web.build-test-results-tab .

Thanks for re-running the check - I also suspect this might be related to RequiresProcessIsolation; there's not much else that is different in these tests. We'll see if that's the case soon enough...

MichalStrehovsky · 2024-03-27T08:55:53Z

Can anyone sign off? It's not acceptable to have main in a state where we basically don't run nativeaot testing for a week.

This needs to be merged before #100331 can fix testing RequiresProcessIsolation tests.

The test failure is an unrelated GC issue.

MichalStrehovsky · 2024-03-27T11:32:16Z

Thanks!

This is unfortunate because it was added for native AOT sake in dotnet#91415. Then Tomas rewrote the test to execute corerun.exe in dotnet#91560 making it 100% unsupportable with native AOT. What the test is doing doesn't look supportable with the merged runner infra and runs into the same issues I saw in dotnet#100269. We never noticed this test is broken because tests requiring process isolation are completely busted with native AOT. I did a local run and started re-baselining again because this is not the only regression.

This is unfortunate because it was added for native AOT sake in #91415. Then Tomas rewrote the test to execute corerun.exe in #91560 making it 100% unsupportable with native AOT. What the test is doing doesn't look supportable with the merged runner infra and runs into the same issues I saw in #100269. We never noticed this test is broken because tests requiring process isolation are completely busted with native AOT. I did a local run and started re-baselining again because this is not the only regression.

MichalStrehovsky · 2024-03-26T06:55:04Z

There are two issues with this:

1. The merged tests are actually broken, but the failures are only observable without Helix

Helix does not report the failures (this is the reason I’m doing a rollback right away instead of waiting around for a fix). Example:

runtime/src/tests/nativeaot/SmokeTests/Exceptions/Exceptions.cs

Lines 67 to 71 in 78a895f

if(!stackTrace.Contains("BringUpTest.Main"))
{
Console.WriteLine("Unexpected stack trace: "+stackTrace);
returnFail;
}

The above should obviously fail because there is no BringUpTest.Main (it got renamed to BringUpTest.TestEntryPoint as part of merging). The CI is all green, despite of this. The test fails if I run it locally through:

build clr.aot+libs -rc Checked -lc Release
src\tests\build.cmd nativeaot checked tree nativeaot\SmokeTests
src\tests\run.cmd runnativeaottests checked

2. There are several test failures, some of which look like they’ll need fixes in the merged wrapper infrastructure

The Exceptions test wants to be able to test AppDomain.UnhandledException:
runtime/src/tests/nativeaot/SmokeTests/Exceptions/Exceptions.cs
Line 43 in 78a895f
AppDomain.CurrentDomain.UnhandledException+=UnhandledExceptionEventHandler;

The wrapper however generates a Try/Catch that doesn’t let UnhandledException happen. The test fails.
The TrimmingBehaviors test is sensitive to ripple effects in whole program view. The wrapper’s use of Xunit Assert.Equal brings in a massive amount of reflection that causes ripple effects all over the place. The test fails in a rather obscure way and it took a while to root cause.

I’m also concerned about whether we have any tests that may now be succeeding because of the ripple effects from the wrapper (and we would not notice if the thing they are testing gets broken because they get saved by the ripple effects.

To fix this one, can we make it so RequiresProcessIsolation really just generates a Main that calls the other Main, without checking exit codes, wrapping anything, Assert.Equaling etc?

Cc @dotnet/ilc-contrib

This reverts commit 43b55fa.

dotnet#98469)" This reverts commit 41c6057.

dotnet-policy-service · 2024-03-26T06:55:45Z

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

jkotas · 2024-03-26T11:19:22Z

Helix does not report the failures

Does this affect all merged tests or just the nativeaot ones that you are reverting?

MichalStrehovsky · 2024-03-26T11:31:07Z

Helix does not report the failures
Does this affect all merged tests or just the nativeaot ones that you are reverting?

I'm afraid of the answer to this. I didn't check. Smoke tests are absolutely critical.

jkotas · 2024-03-27T00:29:41Z

I'm afraid of the answer to this. I didn't check.

Checking: #100319

agocke · 2024-03-27T01:24:01Z

The wrapper’s use of Xunit Assert.Equal brings in a massive amount of reflection that causes ripple effects all over the place.

This shouldn't be true anymore. The Microsoft.Dotnet.XUnitAssert package should sub in and it should be trim safe.

jkotas · 2024-03-27T03:54:50Z

I'm afraid of the answer to this. I didn't check.

The other runtime tests are failing as expected https://dev.azure.com/dnceng-public/public/_build/results?buildId=620112&view=ms.vss-test-web.build-test-results-tab .

MichalStrehovsky · 2024-03-27T05:21:01Z

The wrapper’s use of Xunit Assert.Equal brings in a massive amount of reflection that causes ripple effects all over the place.
This shouldn't be true anymore. The Microsoft.Dotnet.XUnitAssert package should sub in and it should be trim safe.

Trim safety is not the problem here, it's the use of reflection on random stuff in the BCL.

In the case I saw, somewhere after Assert.Equal<T> we marked methods on IEquatable<T> as reflected on and that was breaking what a test was trying to exercise because we were no longer able to remove something. (It took extra long to troubleshoot because I thought it's something I broke, only to find out the test is broken in main.)

The tests under nativeaot need to have very precise control over what's in the dependency graph to be able to test the things they are testing. The implementation of Assert.Equal that still depends on reflection doesn't feel like what we'd want in the runtime test tree (it's fine for libs testing if it's trim safe, but runtime tests should have as little code running as part of the test infrastructure as possible).

MichalStrehovsky · 2024-03-27T05:23:47Z

I'm afraid of the answer to this. I didn't check.
The other runtime tests are failing as expected https://dev.azure.com/dnceng-public/public/_build/results?buildId=620112&view=ms.vss-test-web.build-test-results-tab .

Thanks for re-running the check - I also suspect this might be related to RequiresProcessIsolation; there's not much else that is different in these tests. We'll see if that's the case soon enough...

MichalStrehovsky · 2024-03-27T08:55:53Z

Can anyone sign off? It's not acceptable to have main in a state where we basically don't run nativeaot testing for a week.

This needs to be merged before #100331 can fix testing RequiresProcessIsolation tests.

The test failure is an unrelated GC issue.

MichalStrehovsky · 2024-03-27T11:32:16Z

Thanks!

This is unfortunate because it was added for native AOT sake in dotnet#91415. Then Tomas rewrote the test to execute corerun.exe in dotnet#91560 making it 100% unsupportable with native AOT. What the test is doing doesn't look supportable with the merged runner infra and runs into the same issues I saw in dotnet#100269. We never noticed this test is broken because tests requiring process isolation are completely busted with native AOT. I did a local run and started re-baselining again because this is not the only regression.

This is unfortunate because it was added for native AOT sake in #91415. Then Tomas rewrote the test to execute corerun.exe in #91560 making it 100% unsupportable with native AOT. What the test is doing doesn't look supportable with the merged runner infra and runs into the same issues I saw in #100269. We never noticed this test is broken because tests requiring process isolation are completely busted with native AOT. I did a local run and started re-baselining again because this is not the only regression.

MichalStrehovsky · 2024-03-26T06:55:04Z

There are two issues with this:

1. The merged tests are actually broken, but the failures are only observable without Helix

Helix does not report the failures (this is the reason I’m doing a rollback right away instead of waiting around for a fix). Example:

runtime/src/tests/nativeaot/SmokeTests/Exceptions/Exceptions.cs

Lines 67 to 71 in 78a895f

if(!stackTrace.Contains("BringUpTest.Main"))
{
Console.WriteLine("Unexpected stack trace: "+stackTrace);
returnFail;
}

The above should obviously fail because there is no BringUpTest.Main (it got renamed to BringUpTest.TestEntryPoint as part of merging). The CI is all green, despite of this. The test fails if I run it locally through:

build clr.aot+libs -rc Checked -lc Release
src\tests\build.cmd nativeaot checked tree nativeaot\SmokeTests
src\tests\run.cmd runnativeaottests checked

2. There are several test failures, some of which look like they’ll need fixes in the merged wrapper infrastructure

The Exceptions test wants to be able to test AppDomain.UnhandledException:
runtime/src/tests/nativeaot/SmokeTests/Exceptions/Exceptions.cs
Line 43 in 78a895f
AppDomain.CurrentDomain.UnhandledException+=UnhandledExceptionEventHandler;

The wrapper however generates a Try/Catch that doesn’t let UnhandledException happen. The test fails.
The TrimmingBehaviors test is sensitive to ripple effects in whole program view. The wrapper’s use of Xunit Assert.Equal brings in a massive amount of reflection that causes ripple effects all over the place. The test fails in a rather obscure way and it took a while to root cause.

I’m also concerned about whether we have any tests that may now be succeeding because of the ripple effects from the wrapper (and we would not notice if the thing they are testing gets broken because they get saved by the ripple effects.

To fix this one, can we make it so RequiresProcessIsolation really just generates a Main that calls the other Main, without checking exit codes, wrapping anything, Assert.Equaling etc?

Cc @dotnet/ilc-contrib

This reverts commit 43b55fa.

dotnet#98469)" This reverts commit 41c6057.

dotnet-policy-service · 2024-03-26T06:55:45Z

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

jkotas · 2024-03-26T11:19:22Z

Helix does not report the failures

Does this affect all merged tests or just the nativeaot ones that you are reverting?

MichalStrehovsky · 2024-03-26T11:31:07Z

Helix does not report the failures
Does this affect all merged tests or just the nativeaot ones that you are reverting?

I'm afraid of the answer to this. I didn't check. Smoke tests are absolutely critical.

jkotas · 2024-03-27T00:29:41Z

I'm afraid of the answer to this. I didn't check.

Checking: #100319

agocke · 2024-03-27T01:24:01Z

The wrapper’s use of Xunit Assert.Equal brings in a massive amount of reflection that causes ripple effects all over the place.

This shouldn't be true anymore. The Microsoft.Dotnet.XUnitAssert package should sub in and it should be trim safe.

jkotas · 2024-03-27T03:54:50Z

I'm afraid of the answer to this. I didn't check.

The other runtime tests are failing as expected https://dev.azure.com/dnceng-public/public/_build/results?buildId=620112&view=ms.vss-test-web.build-test-results-tab .

MichalStrehovsky · 2024-03-27T05:21:01Z

The wrapper’s use of Xunit Assert.Equal brings in a massive amount of reflection that causes ripple effects all over the place.
This shouldn't be true anymore. The Microsoft.Dotnet.XUnitAssert package should sub in and it should be trim safe.

Trim safety is not the problem here, it's the use of reflection on random stuff in the BCL.

In the case I saw, somewhere after Assert.Equal<T> we marked methods on IEquatable<T> as reflected on and that was breaking what a test was trying to exercise because we were no longer able to remove something. (It took extra long to troubleshoot because I thought it's something I broke, only to find out the test is broken in main.)

The tests under nativeaot need to have very precise control over what's in the dependency graph to be able to test the things they are testing. The implementation of Assert.Equal that still depends on reflection doesn't feel like what we'd want in the runtime test tree (it's fine for libs testing if it's trim safe, but runtime tests should have as little code running as part of the test infrastructure as possible).

MichalStrehovsky · 2024-03-27T05:23:47Z

I'm afraid of the answer to this. I didn't check.
The other runtime tests are failing as expected https://dev.azure.com/dnceng-public/public/_build/results?buildId=620112&view=ms.vss-test-web.build-test-results-tab .

Thanks for re-running the check - I also suspect this might be related to RequiresProcessIsolation; there's not much else that is different in these tests. We'll see if that's the case soon enough...

MichalStrehovsky · 2024-03-27T08:55:53Z

Can anyone sign off? It's not acceptable to have main in a state where we basically don't run nativeaot testing for a week.

This needs to be merged before #100331 can fix testing RequiresProcessIsolation tests.

The test failure is an unrelated GC issue.

MichalStrehovsky · 2024-03-27T11:32:16Z

Thanks!

This is unfortunate because it was added for native AOT sake in dotnet#91415. Then Tomas rewrote the test to execute corerun.exe in dotnet#91560 making it 100% unsupportable with native AOT. What the test is doing doesn't look supportable with the merged runner infra and runs into the same issues I saw in dotnet#100269. We never noticed this test is broken because tests requiring process isolation are completely busted with native AOT. I did a local run and started re-baselining again because this is not the only regression.

This is unfortunate because it was added for native AOT sake in #91415. Then Tomas rewrote the test to execute corerun.exe in #91560 making it 100% unsupportable with native AOT. What the test is doing doesn't look supportable with the merged runner infra and runs into the same issues I saw in #100269. We never noticed this test is broken because tests requiring process isolation are completely busted with native AOT. I did a local run and started re-baselining again because this is not the only regression.

MichalStrehovsky added 3 commits March 26, 2024 07:47

Revert "Add back NoWarn (dotnet#99995)"
2458c3f
This reverts commit 43b55fa.

Revert "Convert the Remaining Legacy Tests to the Merged Wrapper Model (
ecf2d58
dotnet#98469)" This reverts commit 41c6057.

Keep the non-nativeaot changes
5d7a9ad

MichalStrehovsky requested review from ivdiazsa and jkoritzinsky March 26, 2024 06:55

ghost added the area-NativeAOT-coreclr label Mar 26, 2024

dotnet-policy-serviceBot assigned MichalStrehovsky Mar 26, 2024

MichalStrehovsky mentioned this pull request Mar 26, 2024
Fix regression from test merging #99996
Closed

build-analysisBot mentioned this pull request Mar 26, 2024
Assert failure(PID 13812 [0x000035f4], Thread: 14128 [0x3730]): promoted_bytes (heap_number) == promoted #100035
Closed

vitek-karas approved these changes Mar 27, 2024
View reviewed changes

MichalStrehovsky merged commit 782c2ee into dotnet:mainMar 27, 2024

MichalStrehovsky deleted the rollb branch March 27, 2024 11:32

MichalStrehovsky mentioned this pull request Apr 3, 2024
Exclude unhandledTester test on native AOT #100574
Merged

github-actionsBot locked and limited conversation to collaborators Apr 27, 2024