Repository navigation
Various BenchmarksGame tests failing due to inability to resolve a manifest resource stream #118475
Description
Various tests under JIT/Performance/CodeQuality/BenchmarksGame including k-nucleotide-9, k-nucleotide-1, reverse-complement-1, regex-reduce-1, and regex-redux-5 are failing as the manifest resource stream is returning null.
For example, regex-reduce-5 uses the following and the result of GetInputStream is null:
class TestHarnessHelpers
{
public readonly int ExpectedLength;
private readonly string resourceName;
public TestHarnessHelpers(bool bigInput, [System.Runtime.CompilerServices.CallerFilePath] string csFileName = "")
{
if (bigInput)
{
ExpectedLength = 136381;
resourceName = $"{Path.GetFileNameWithoutExtension(csFileName)}.regexdna-input25000.txt";
}
else
{
ExpectedLength = 152;
resourceName = $"{Path.GetFileNameWithoutExtension(csFileName)}.regexdna-input25.txt";
}
}
public Stream GetInputStream()
{
var assembly = typeof(TestHarnessHelpers).GetTypeInfo().Assembly;
return assembly.GetManifestResourceStream(resourceName);
}
}Reacted by Copilot
Activity
tannergooding commentedon Aug 7, 2025
This was found in #117845
Disable some tests due to [ActiveIssue("#118475")]
Also mark reverse-complement-6 for [ActiveIssue("#118475")]
[main] Source code updates from dotnet/dotnet (#117845)
dotnet-policy-service commentedon Aug 7, 2025
Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.
dotnet-policy-service commentedon Aug 7, 2025
Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.
jkotas commentedon Aug 8, 2025
This is regression introduced by dotnet/sdk#49328 that changed the default value of RootNamespace property for projects that contain
-and other special characters in the names.The RootNamespace property is used to compute the resource names. The resource name that these tests expect is
k-nucleotide-9.knucleotide-input.txt, but the actual name produced by .NET SDK after the above change isk_nucleotide_9.knucleotide-input.txt. Notice-vs._in the name.@baronfel @marcpopMSFT Is this an expected .NET SDK breaking change?
jkotas commentedon Aug 8, 2025
For reference, RootNamespace is used as an input for resource name generation here: https://github.com/dotnet/msbuild/blob/main/src/Tasks/Microsoft.CSharp.CurrentVersion.targets#L102
marcpopMSFT commentedon Aug 11, 2025
We did this change at the request of a customer for us to match what the template engine already does (ie the template engine already rewrites that value when creating a new project but a customer trying manually changing it and spent a bunch of time investigating something that went wrong). The Rootnamespace is used when creating new classes by the template engine and VS and those scenarios are broken if invalid characters are used.
It's a fair take that this code be breaking for customers that have manually set that property and are using very narrow workflows that don't run into the situations that are broken. We're currently discussing if it's a significant enough break to document or tie to an sdk analysis level. Am I correct in assuming it's a quick fix in the tests?
tannergooding commentedon Aug 11, 2025
Shouldn't be difficult to fix in the tests, no. Just adjusting to match the new convention.
jkotas commentedon Aug 11, 2025
Yes, we can certainly fix the tests.
I think more likely case are customers who renamed their project to have a dash in the name (or variants of it like started with a sample and rename it - it is effectively how we ended up with the problem in our tests):
jkotas commentedon Aug 11, 2025
To fix this:
[ActiveIssue("https://github.com/dotnet/runtime/issues/118475")]to reenable the tests-to_in the first part of the embedded resource names used by these tests. For example, changek-nucleotide-9.knucleotide-input.txttok_nucleotide_9.knucleotide-input.txtjkotas commentedon Aug 11, 2025
@marcpopMSFT It occurred to me that the fix is incomplete. There are many other characters in addition to
-that are invalid in C# identifiers that the fix does not handle.In my opinion, the fix in SDK should be reverted. It is needless breaking change and it is incomplete. The template engine in VS should be fixed instead.
Fix benchmarksgame tests to be able to resolve the embedded resources (…
marcpopMSFT commentedon Aug 12, 2025
The code already had a special case for spaces. This was done based on the customer feedback ticket that requested we do this centrally so that each consumer of rootnamespace didn't have to handle the invalid characters. We can't handle every possible invalid character in msbuild but it seemed worth it to solve for a few additional easy to identify cases. https://developercommunity.visualstudio.com/t/Visual-Studio-does-not-automatically-cre/10770251
It sounds like there are some cases where invalid characters in the root namespace are actually allowed which ends up making it breaking which is unexpected (it looks like resource scenarios are one of those cases). 305k csproj files on GH have a dash but 299k of them have a root namespace so the impact is likely low but not zero.
We're still discussing whether to document or revert or other.
jkotas commentedon Aug 12, 2025
How many of these consumers are there?
How many .csproj files on GH include other characters that are invalid in C# identifiers, and thus still exposed to this bug? I have in mind all those names with emojis that are popular these days. To make it concrete, this is still broken:
I♥.NETtannergooding commentedon Aug 13, 2025
Can we not have something like
RootNamespaceandRootNamespaceName(orRootNamespaceId,RootNamespaceMetadataName, etc) to differentiate the two?We notably do splits like this in a few places, explicitly to handle nuances like "here's what the user input and here's what the SDK or other tools actually need to use". Sometimes these are internal splits (i.e.
Fooand_Foo) and other times they are split by a minor suffix or prefix difference (i.e.FooandFooBar).This seems like a "cleaner" option that doesn't risk breaking existing things that use
RootNamespaceand which allows other things to migrate to a newer option that does any "fixups" expected for the name to be compatible with C# naming conventions (notablyILdoesn't have the same restrictions; spaces, emoji, and other characters are valid in IL and some languages like F# have syntax to allow code to use such things too).marcpopMSFT commentedon Aug 15, 2025
I do not know how many consumers of rootnamespace are out there and you're correct that I risk breaking them even if the current value is invalid as we are aware of it being used. I don't want to try to tackle this for other characters as we decided to do this because there was already prior art in the SDK to handle " " characters. I don't think it's worth coming up with a completely separate concept that consumers could use that would separate the sanitized version of this versus not though that's an interesting suggestion to consider.
Ultimately, we were just trying to handle a few simple cases we knew were not supported. I'll consult with @baronfel next week but I don't think we want to do more here and I'm not sure we'd decide to revert either.