Skip to content

Rewrite UTF8 string marshalling test suite with Theory methods and bool-returning native functions#126366

Merged
jkoritzinsky merged 3 commits intomainfrom
copilot/rewrite-utf8test-suite
Apr 2, 2026
Merged

Rewrite UTF8 string marshalling test suite with Theory methods and bool-returning native functions#126366
jkoritzinsky merged 3 commits intomainfrom
copilot/rewrite-utf8test-suite

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 31, 2026

Modernises the UTF8 string marshalling interop tests by splitting the single monolithic [Fact] into parameterised [Theory] tests, replacing native throw with BOOL return values, and consolidating all test classes into StringMarshaling.UTF8Tests.

Description

C# (UTF8Test.cs)

  • Merged UTF8StringTests, UTF8StringBuilderTests, UTF8StructMarshalling, UTF8DelegateMarshalling, and Test into a single StringMarshaling.UTF8Tests class
  • Replaced the single TestEntryPoint loop-driven [Fact] with individual [Theory] methods backed by Utf8StringsWithIndex() / NonNullUtf8StringsWithIndex() static [MemberData] sources
  • Updated DllImport declarations for StringParameterRef, StringBuilderParameterInOut, and TestStructWithUtf8Field to return bool with [return: MarshalAs(UnmanagedType.Bool)]
  • All assertions use Assert.* instead of throw new Exception
  • Split TestUTF8StructMarshalling into TestStructWithUtf8FieldParameter ([Theory]), TestSetStringInStruct ([Fact]), and CompareWithUTF8Encoding ([Fact], non-Windows only)
  • Fixed TestOutStringParameter to call StringParameterOut (was incorrectly calling StringParameterInOut)
  • TestEmptyString now asserts the returned string is null (empty input → native returns null pointer)

Native (UTF8TestNative.cpp)

  • StringBuilderParameterInOut, TestStructWithUtf8Field, StringParameterRef: voidBOOL; validation failures return FALSE instead of throw
  • utf16_to_utf8 / utf8_to_utf16 helpers: throw on conversion failure → return nullptr; allocated buffers are now freed before returning nullptr on the error path
  • Fixed memory leaks on all FALSE/nullptr error paths: utf16_to_utf8, utf8_to_utf16, StringBuilderParameterInOut, TestStructWithUtf8Field, and StringParameterRef all free their allocated buffers before returning on failure

Before / After

// Before: one Fact running everything in loops
[Fact]
public static void TestEntryPoint() {
    for (int i = 0; i < utf8Strings.Length; i++)
        UTF8StringTests.TestInOutStringParameter(utf8Strings[i], i);
    // ...
}

// After: individual Theory per scenario
[Theory]
[MemberData(nameof(Utf8StringsWithIndex))]
public static void TestInOutStringParameter(string orgString, int index)
{
    string nativeString = StringParameterInOut(orgString, index);
    Assert.Equal(orgString, nativeString);
}

Copilot AI and others added 2 commits March 31, 2026 18:21
…Tests class

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/e82c1a51-21a5-4efc-b0e3-9cead95daa4d

Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
…assertions, use nullptr in helpers

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/e82c1a51-21a5-4efc-b0e3-9cead95daa4d

Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

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

@jkoritzinsky jkoritzinsky marked this pull request as ready for review March 31, 2026 22:09
Copilot AI review requested due to automatic review settings March 31, 2026 22:09
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR modernizes the UTF-8 string marshalling interop tests by replacing a single loop-driven entry-point test with granular [Theory]-based cases, while updating native helpers to report validation failures via BOOL return values instead of throwing.

Changes:

  • Consolidated multiple UTF-8 marshalling test classes into StringMarshaling.UTF8Tests and split scenarios into parameterized [Theory] tests using [MemberData].
  • Updated several native exports to return BOOL for validation success/failure and adjusted corresponding C# P/Invokes to return bool.
  • Corrected and clarified a few test behaviors (e.g., TestOutStringParameter calling the intended native export; empty string returning null).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
src/tests/Interop/StringMarshalling/UTF8/UTF8Test.cs Refactors the managed test suite into theory-based tests and updates P/Invoke signatures to match new native return values.
src/tests/Interop/StringMarshalling/UTF8/UTF8TestNative.cpp Changes select native test entrypoints/helpers to return status codes instead of throwing, and adjusts conversion helpers to return nullptr on failure.

@jkoritzinsky
Copy link
Copy Markdown
Member

@copilot apply changes based on the comments in this thread

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/c9e17223-74a0-41ef-b260-01918d68a88d

Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 2, 2026

@copilot apply changes based on the comments in this thread

Fixed in 911d512. All five memory leaks on error paths are now addressed:

  • utf16_to_utf8: CoreClrFree(pszUTF8) before return nullptr
  • utf8_to_utf16: CoreClrFree(wszTextUTF16) before return nullptr
  • StringBuilderParameterInOut: free_utf8_string(pszTextutf8) before return FALSE
  • TestStructWithUtf8Field: free_utf8_string(pszNative) before return FALSE
  • StringParameterRef: free_utf8_string(pszTextutf8) before return FALSE

@jkoritzinsky jkoritzinsky merged commit 3338406 into main Apr 2, 2026
82 checks passed
@jkoritzinsky jkoritzinsky deleted the copilot/rewrite-utf8test-suite branch April 2, 2026 21:45
@github-project-automation github-project-automation bot moved this to Done in AppModel Apr 2, 2026
@MichalStrehovsky
Copy link
Copy Markdown
Member

This change broke all the outerloops again.

artifacts/tests/coreclr/obj/AnyOS.x64.Checked/Managed/Interop/Interop/generated/XUnitWrapperGenerator/XUnitWrapperGenerator.XUnitWrapperGenerator/FullRunner.g.cs(9181,118): error CS0103: The name 'testArguments' does not exist in the current context [/__w/1/s/src/tests/Interop/Interop.csproj]

Revert at #126498

MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this pull request Apr 3, 2026
…s and bool-returning native functions (dotnet#126366)"

This reverts commit 3338406.
MichalStrehovsky added a commit that referenced this pull request Apr 3, 2026
…s and bool-returning native functions" (#126498)

Unbreaks outerloops.

Reverts #126366
@jkoritzinsky
Copy link
Copy Markdown
Member

Looks like the generated code for XUnitWrapperGenerator for handling Theory dynamic member data is broken.

radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Apr 9, 2026
…ol-returning native functions (dotnet#126366)

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Apr 9, 2026
…s and bool-returning native functions" (dotnet#126498)

Unbreaks outerloops.

Reverts dotnet#126366
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done
Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants