Fix test containing reflection erroring from #117168 (Add methods from #27912 (Flow System.Text.Rune through more APIs))

[Joy-less]

Fixes alleged test errors from #117168 for the CreateBroadcasting_AllMethodsOverridden test, which uses reflection to check that the new overloads are overridden in NullTextWriter.

[Copilot]

Pull Request Overview

This PR fixes test failures in the CreateBroadcasting_AllMethodsOverridden test by adding missing Rune parameter overrides to the NullTextWriter class. The test uses reflection to verify that all methods from the base TextWriter class are properly overridden, and was failing because new Rune-based methods introduced in PR #117168 were not implemented in NullTextWriter.

• Adds four missing Rune parameter method overrides to NullTextWriter
• Ensures all new Rune-based methods from the base class have corresponding implementations

[Joy-less]

I added overrides to SyncTextWriter too, since I noticed those were also missing, although those shouldn't affect the tests.

[jkotas]

It is still failing with these fixes.

[Joy-less]

It is still failing with these fixes.

My apologies, I added the overrides to NullTextWriter and SyncTextWriter but not to BroadcastingTextWriter. I've done a search for classes inheriting from TextWriter to make sure I haven't missed any others, and the other classes inheriting from TextWriter don't implement every write overload. So it should work now.

Edit: The tests will not display as successful because you reverted the original changes.

[jkotas]

Could you please force push revert of the revert into this PR? This PR should have two commits: revert of the revert, fixes.

[dotnet-policy-service]

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

[tarekgh]

I am seeing more tests are subclassing TextWriting, https://github.com/dotnet/runtime/blob/main/src/libraries/Common/tests/System/Xml/ModuleCore/cltmconsole.cs is example but there are more there. I am not sure if these tests will fail though. I'll try to look at these tests and update them if needed.

[jkotas]

/azp run runtime-nativeaot-outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jkotas]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Joy-less]

I am seeing more tests are subclassing TextWriting, https://github.com/dotnet/runtime/blob/main/src/libraries/Common/tests/System/Xml/ModuleCore/cltmconsole.cs is example but there are more there. I am not sure if these tests will fail though. I'll try to look at these tests and update them if needed.

The only one that is actually tested to my knowledge is BroadcastingTextWriter (at first glance I thought it was NullTextWriter). I have added the overloads to NullTextWriter, SyncTextWriter and BroadcastingTextWriter since all three of these implement every overload. All other TextWriter-inheriting classes that I could find do not implement every overload, including the one you linked.

[tarekgh]

I am seeing

runtime/src/libraries/System.IO/src/System/CodeDom/Compiler/IndentedTextWriter.cs

Line 12 in 082bc29

public class IndentedTextWriter : TextWriter

.

@dotnet/area-system-codedom We are adding support to write Rune in TextWriter and I came across CodeDom IndentedTextWriter. Do you know how interesting supporting Rune writing in there? I think it will be a simple to do for completeness but wanted to check with you first.

[stephentoub]

Do you know how interesting supporting Rune writing in there? I think it will be a simple to do for completeness but wanted to check with you first.

Adding an override to IndentedTextWriter would "just" be about performance, correct? Whether or not we add an override, the functional results will be the same, but adding the override would make it more efficient when used, yes?

[tarekgh]

Adding an override to IndentedTextWriter would "just" be about performance, correct? Whether or not we add an override, the functional results will be the same, but adding the override would make it more efficient when used, yes?

IndentedTextWriter is indenting before writing, here is example of writing char

        public override void Write(char value)
        {
            OutputTabs();
            _writer.Write(value);
        }

So, it is functionality correctness too.

[stephentoub]

So, it is functionality correctness too.

If that's true, that means the new virtual method we added is buggy. We have to expect that the new method will be used with derived writers that won't yet have overridden it, and it should behave identically to if someone used whatever equivalent APIs would have been used before Rune was available.

That said, without diving deeper, I don't see why it'd be a functional issue. OutputTabs doesn't add tabs on each call, only if it's the beginning of a new line.

[tarekgh]

If that's true, that means the new virtual method we added is buggy.

What do you mean? I don't think the TextWriter new write Rune virtual methods are buggy. IndentedTextWriter subclass TextWriter to indent text before writing. To do so, it has to override the Write methods.

[tarekgh]

That said, without diving deeper, I don't see why it'd be a functional issue. OutputTabs doesn't add tabs on each call, only if it's the beginning of a new line.

What happens if writing Rune in the beginning of the new line?

[stephentoub]

What happens if writing Rune in the beginning of the new line?

The same thing that happens when writing a char in the beginning of the new line, no?

[tarekgh]

The same thing that happens when writing a char in the beginning of the new line, no?

You are right, writing Rune will invoke the char overload which should call the overridden one from IndentedTextWriter and not from the base class TextWriter. So, we should be fine then doing nothing.

[stephentoub]

It'd still be good to add the overload/override, but the CodeDom projects builds downlevel as well, so the overload will be ifdef'd, and that's fine because it doesn't change functionality, only performance.

[tarekgh]

@stephentoub one more question, running System.IO.Tests, there is one unrelated failure in

runtime/src/libraries/System.Runtime/tests/System.IO.Tests/MemoryStream/MemoryStreamTests.cs

Line 163 in f610923

Assert.Throws<ArgumentOutOfRangeException>(() => ms.Capacity = MaxSupportedLength + 1);

. The test expect ArgumentOutOfRangeException while it throws OOM exception. I think Capacity property in MemryStream is missing the check:

                    if (value > MemStreamMaxLength)
                    {
                        throw new ArgumentOutOfRangeException(nameof(value), SR.ArgumentOutOfRange_StreamLength);
                    }

should I add this to this PR? or submit it in a different PR

[jkotas]

one more question, running System.IO.Tests, there is one unrelated failure

#120091

[tarekgh]

@stephentoub

It'd still be good to add the overload/override, but the CodeDom projects builds downlevel as well, so the overload will be ifdef'd, and that's fine because it doesn't change functionality, only performance.

We don't need to ifdef anything here because on the type is defined in the assemblies: netstandard.dll, System.Runtime.dll. So, changing this type will be only observed in 10. Thanks to @ericstj helping to clarify that. I'll go ahead and push the changes to the IndentedTextWriter.