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

🤖 Copilot Code Review — PR #126412

Holistic Assessment

Motivation: This PR migrates Mono.Linker.Tests from NUnit to MSTest as part of the broader dotnet/runtime effort to standardize on MSTest. The motivation is clear and the change is justified.

Approach: The migration is thorough and mechanical — attribute substitutions ([TestFixture]→[TestClass], [Test]→[TestMethod], [TestCaseSource]→[DynamicData]), assertion API changes, and data source type changes (TestCaseData→TestDataRow<T>). The approach is sound.

Summary: ✅ LGTM. The migration is comprehensive and correct. All NUnit assertion methods are properly mapped to their MSTest equivalents with correct argument ordering (verified against the MSTest source). Two minor style observations noted below, neither blocking.

Detailed Findings

✅ Assertion API Correctness — Verified against MSTest source

All assertion conversions were verified against the actual MSTest source code:

• Assert.IsLessThan(expectedMaxLines, lineCount, msg) (IndividualTests.cs:198): Correct. MSTest signature is IsLessThan(T upperBound, T value) — asserts value < upperBound, so this correctly asserts lineCount < expectedMaxLines.
• Assert.Contains(item, collection) / Assert.DoesNotContain(item, collection) (ResultChecker.cs:841,849): Correct. MSTest signature is Contains<T>(T expected, IEnumerable<T> collection).
• Assert.IsEmpty(collection, msg) (ResultChecker.cs:864, TestFrameworkRulesAndConventions.cs): Correct. Exists in Assert.Count.cs.
• Assert.Inconclusive replacing NUnit Assert.Ignore: Correct MSTest equivalent.
• UnitTestAssertException replacing NUnit's individual exception types (TestRunner.cs:32): Correct — MSTest's base exception class covers all assertion failure types.

✅ Data Source Migration — Correct

• [TestCaseSource(typeof(T), nameof(...))] → [DynamicData(nameof(...), typeof(T))]: Argument order correctly swapped (MSTest puts name first, type second).
• [DynamicData(nameof(Method), new object[] { typeof(T) })] for data source methods that take parameters: Uses the MSTest constructor for passing method arguments.
• TestCaseData → TestDataRow<T> with tuple unpacking for multi-parameter test methods: Correct.

✅ Attribute Migration — Complete

All NUnit attributes properly mapped: [TestFixture]→[TestClass], [Test]→[TestMethod], [SetUp]→[TestInitialize], [TestCase]→[DataRow], [NonParallelizable]→[DoNotParallelize], [Parallelizable]→[Parallelize]. No remaining NUnit references in .cs files.

💡 Using Directive Ordering — MemberAssertionsCollector.cs

In MemberAssertionsCollector.cs (lines 8-9), the Microsoft.VisualStudio.TestTools.UnitTesting using comes after Mono.Cecil. Alphabetically, Microsoft should sort before Mono. All other files in this PR have the correct ordering.

// Current (lines 8-9):
using Mono.Cecil;
using Microsoft.VisualStudio.TestTools.UnitTesting;

// Suggested:
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Mono.Cecil;

💡 CollectionAssert.AreEquivalent Argument Order — Convention

Several CollectionAssert.AreEquivalent calls place the actual value first and expected second (e.g., IndividualTests.cs:78-80, ResultChecker.cs:831-833). MSTest's signature is AreEquivalent(ICollection expected, ICollection actual). Since AreEquivalent checks bidirectional membership, this doesn't affect correctness — only error message clarity on failure. Non-blocking, but worth noting for consistency.

Generated by Code Review for issue #126412 · ◷

I considered that, but it's way easier to move to MSTest as the API surface is quite close to NUnit, while very different for xUnit and will require more efforts.

/ba-g failing jobs don't run this test suite, unrelated