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.

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

Fix regression from test merging #99996

Closed
wants to merge 2 commits into from

Conversation

MichalStrehovsky
Copy link
Member

#98469 regressed a development scenario around passing command line arguments to this test. Command line argument can selectively choose one test to run. This is extremely useful when there’s test failures. The test does a ton of things and ability to zoom in is useful.

Alternatively, we could put the test on the non-merged plan, same way we left nativeaot\SmokeTests\ControlFlowGuard on the non-merged plan (I assume for the same reason because it uses command line arguments?). I would prefer that solution but made a PR for the other approach because if I understand it correctly, we don't want to leave tests like ControlFlowGuard.

dotnet#98469 regressed a development scenario around passing command line arguments to this test. Command line argument can selectively choose one test to run. This is extremely useful when there’s test failures. The test does a ton of things and ability to zoom in is useful.

Alternatively, we could put the test on the non-merged plan, same way we left nativeaot\SmokeTests\ControlFlowGuard on the non-merged plan (I assume for the same reason because it uses command line arguments?). I would prefer that solution but made a PR for the other approach because if I understand it correctly, we don't want to leave tests like that.
Copy link
Contributor

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

Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

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

LGTM. In the future, if we convert these tests to use the XUnit attributes, we can get the filtering support that the source generator provides.

@MichalStrehovsky
Copy link
Member Author

LGTM. In the future, if we convert these tests to use the XUnit attributes, we can get the filtering support that the source generator provides.

Where is the filtering? I actually wanted to delete DynamicGenerics.main.cs and just put [Fact] on everything, but the generated C# seems to be just calling everything sequentially and that's a regression from the existing harness can do (it allows filtering, and it also lets the tests continue if one of them throws an exception/fails).

@jkoritzinsky
Copy link
Member

Right now the tests are all marked as process-isolated and use the standalone mode. If they weren't process-isolated, then the root NativeAot merged runner would generate the Main with filtering and specific result support.

@MichalStrehovsky
Copy link
Member Author

Right now the tests are all marked as process-isolated and use the standalone mode. If they weren't process-isolated, then the root NativeAot merged runner would generate the Main with filtering and specific result support.

Maybe I'm doing something wrong - if I remove the RequiresProcessIsolation from DynamicGenerics.csproj, and replace all [TestMethod] with [Fact] and delete the custom runner, this how how the generated Main looks like:

public static int Main()
{
	try
	{
		My.TestActivatorCreateInstance();
		My.TestDefaultCtorInLazyGenerics();
		global::ArrayTests.ArrayTests.TestArrays();
		global::ArrayTests.ArrayTests.TestDynamicArrays();
		global::ArrayTests.ArrayTests.TestMDArrays();
                // Omitted for brevity
	}
	catch (Exception ex)
	{
		Console.WriteLine(ex.ToString());
		return 101;
	}
	return 100;
}

There's no filtering support and one test failing skips running everything else. Do you know what I missed? I'm not fond of the existing runner because it was pulled out from the .NET Native test tree and it no longer generates things based on [TestMethod]. But we also no longer make changes to it so I don't care too much, but this looked easy enough to just fix.

@jkoritzinsky
Copy link
Member

You should look at the generated main in the NativeAot.csproj project at src/tests/nativeaot.

That's the merged runner project that will have filtering support for individual Fact methods.

@MichalStrehovsky
Copy link
Member Author

You should look at the generated main in the NativeAot.csproj project at src/tests/nativeaot.

Yeah, that one looks good. Except the local workflow is usually not to build everything, but only a subset of the project and that one won't filter. So this fix is still better than converting TestMethod to Fact and deleting the custom runner, at least for now.

However, I'm going to close this PR due to #100269.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 25, 2024
@MichalStrehovsky MichalStrehovsky deleted the dyngen branch April 25, 2024 11:22
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.

None yet

2 participants