[Vinay Kumar Maheshwaram (sudovinay01)]

Summary

When running a test class that extends a test base class, inherited @Test methods were not included in discovery. This could make the child-class run appear successful while silently skipping tests defined by its parent.

This change discovers eligible test methods throughout the superclass hierarchy and lists inherited methods under the child test class.

Changes

• Traverse superclasses when discovering a class’s test methods.
• Skip private superclass methods and avoid duplicate methods when the child overrides a method.
• Associate inherited test items with the child class so they are included when that class is run.
• Add regression coverage for inherited methods, overrides, and child-defined methods.

Gradle source-set configuration and task behavior are unchanged.

Reproducer

https://github.com/sudovinay01/repro-java-issue

Validation

• java-extension/mvnw.cmd clean verify -Declipse.p2.mirrors=false passed.
• The regression test verifies inherited methods are discovered for a child class.
• Manual check in VS Code confirmed running AppMainSourceTestChild executes both the inherited and child-defined methods.

images of issue

### VScode after fix

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[Vinay Kumar Maheshwaram (sudovinay01)]

wenyt (@wenytang-ms) I have addressed your three review comments in commit 17e6b99, added regression coverage, and verified the Java reactor. Could you please take another look when you have a chance?

[wenyt (wenytang-ms)]

If the base class comes from a dependency JAR, running an inherited test like base(TestInfo info) on its own fails here. getCompilationUnit() is null for a binary method, so we get Cannot get compilation unit of methodbase. Running the whole child class avoids this, but running or debugging just that method doesn't.

Could we get the parameter types from an IMethodBinding for binary methods instead of looking for source? We can leave the current source-method path as-is and keep using the child class for execution. A test with a compiled base class and a method parameter would catch this.

[Vinay Kumar Maheshwaram (sudovinay01)]

Addressed in 57344f1. For parameterized JUnit 5/6 methods without a compilation unit, the launch resolver now uses ASTParser.createBindings() to obtain an IMethodBinding and formats its parameter types. The source-backed AST path remains unchanged, and the child class is still used as the execution target. Added a regression with a parent compiled to target/test-classes and a child project referencing it as a binary library; it verifies the selector example.InheritanceBinaryChildTest:base(org.junit.jupiter.api.TestInfo). The full Java reactor passes all 12 plugin tests.

[wenyt (wenytang-ms)]

Problem / Question

1. Duplicate code paths (if (unit == null) vs else) & unnecessary AST traversal:
   ASTParser.createBindings(new IJavaElement[] { method }, new NullProgressMonitor()) works for both source IMethods (unit != null) and binary IMethods (unit == null) in JDT. Keeping the else branch that calls TestSearchUtils.parseToAst(unit, false, ...) + ASTNodeSearchUtil.getMethodDeclarationNode + loops over SingleVariableDeclaration AST nodes is redundant and leaves two parallel paths doing the same job.
2. Missing generic type erasure in formatParameterType:
   When an inherited test method comes from a generic parent class (e.g., abstract class BaseTest<T> { @Test void base(T value) {} }), parameterType.isTypeVariable() is true. In JDT (TypeBinding.getBinaryName()), calling parameterType.getBinaryName() directly on a type variable T returns JDT's internal synthetic key (e.g., pkg.BaseTest$T) instead of the runtime erased JVM class name (java.lang.Object), causing JUnit Platform's DiscoverySelectors.selectMethod to fail with ClassNotFoundException at runtime. Calling parameterType.getErasure() first resolves T to its runtime erased type (java.lang.Object or upper bound).

---

Solution

Unify both source and binary IMethods to use parser.createBindings(new IJavaElement[] { method }, ...) and call parameterType.getErasure() in formatParameterType:

    private String resolveMethodTestName(String handleId, String executionClassName) throws CoreException {
        final IMethod method = (IMethod) JavaCore.create(handleId);
        String testName = method.getElementName();
        if ((this.args.testKind == TestKind.JUnit5 || this.args.testKind == TestKind.JUnit6) &&
                method.getParameters().length > 0) {
            final ASTParser parser = ASTParser.newParser(AST.getJLSLatest());
            parser.setProject(method.getJavaProject());
            final IBinding[] bindings = parser.createBindings(new IJavaElement[] { method },
                    new NullProgressMonitor());
            if (bindings.length != 1 || !(bindings[0] instanceof IMethodBinding)) {
                throw new CoreException(new Status(IStatus.ERROR, JUnitPlugin.PLUGIN_ID, IStatus.ERROR,
                        "Cannot resolve method binding for " + method.getElementName(), null)); //$NON-NLS-1$
            }
            testName += formatParameterTypes((IMethodBinding) bindings[0]);
        }
        final String className = StringUtils.isNotBlank(executionClassName) ? executionClassName :
                method.getDeclaringType().getFullyQualifiedName();
        return className + ':' + testName;
    }

    private String formatParameterTypes(IMethodBinding methodBinding) {
        final List<String> parameters = new LinkedList<>();
        for (final ITypeBinding parameterType : methodBinding.getParameterTypes()) {
            parameters.add(formatParameterType(parameterType));
        }
        return parameters.isEmpty() ? "" : "(" + String.join(",", parameters) + ")";
    }

    private String formatParameterType(ITypeBinding parameterType) {
        final ITypeBinding erasedType = parameterType.getErasure();
        return erasedType.isPrimitive() ? erasedType.getQualifiedName() : erasedType.getBinaryName();
    }

(You can also clean up the unused CompilationUnit, MethodDeclaration, SingleVariableDeclaration, and ASTNodeSearchUtil imports above.)

[Vinay Kumar Maheshwaram (sudovinay01)]

Thanks for the follow-up and the concrete fix. I unified source and binary method parameter resolution through ASTParser.createBindings() and erase parameter types before constructing the JUnit selector. Regression coverage includes a generic source method and a generic method inherited from a compiled parent, preserving the child class as the execution target. I also ran the binary-inheritance fixture in Test Explorer: the inherited method passes on its own, and the complete child class passes 2/2.

[Vinay Kumar Maheshwaram (sudovinay01)]

wenyt (@wenytang-ms) I have also addressed your follow-up about inherited parameterized methods from dependency JARs in commit 57344f1. The new regression uses a compiled parent fixture and verifies the child-class launch selector; the full Java reactor passes all 12 plugin tests. When you have a chance, could you please re-review the updated PR? Thanks.

[Vinay Kumar Maheshwaram (sudovinay01)]

Hi Jinbo Wang (@testforstephen) Changyong Gong (@chagong) wenyt (@wenytang-ms) — could one of you please review this PR when you have time? I’d appreciate feedback on the change and validation, and I’d like to make sure it aligns with the project’s expectations. Thanks for your time and review.

[wenyt (wenytang-ms)]

Same signatures don't necessarily mean an override, e.g. a package-private parent method and a same-named child method in another package. Could we use IMethodBinding.overrides() instead, while preserving the JUnit 4 behavior?

// Keep all more-specific declarations, not one per signature.
final List<IMethodBinding> moreSpecificMethods = new LinkedList<>();
Collections.addAll(moreSpecificMethods, classBinding.getDeclaredMethods());

// Inside the searcher loop, replace the signature-based check:
boolean suppressed = moreSpecificMethods.stream().anyMatch(candidate ->
        candidate.overrides(methodBinding)
        && (searcher.getTestKind() != TestKind.JUnit
            || searcher.isTestMethod(candidate)));
if (suppressed) {
    break;
}

// After processing each superclass, before moving to its parent:
Collections.addAll(moreSpecificMethods, superClass.getDeclaredMethods());

Please add a cross-package regression test and a three-level hierarchy test.

[Vinay Kumar Maheshwaram (sudovinay01)]

Addressed in de1d289. Override suppression now uses IMethodBinding.overrides() across the full hierarchy, while checking that inherited methods are actually accessible from the child (including package-private visibility). Added a cross-package same-signature regression and extended the JUnit 4 fixture to a three-level hierarchy; the existing JUnit 4 unannotated-override behavior is preserved. The Java reactor verification passed after merging upstream/main.

[wenyt (wenytang-ms)]

Thanks for the update! Looking at de1d289, there is a subtle difference between Java's method override rules (IMethodBinding.overrides) and JUnit 5/6's runtime test discovery, as well as a bug in how the for loop checks moreSpecificMethods. Here is a detailed breakdown of the two issues and the exact fix:

1. Problem: isInheritedBy() breaks JUnit 5/6 cross-package package-private @Test methods

In JUnit 5/6 (Jupiter), @Test methods are idiomatic package-private (no public or protected modifier). Even when a child test class is in a different package from its base class, the JUnit Platform runtime still discovers and executes the parent's package-private @Test methods via reflection.

Now consider your test case:

• example.parent.PackagePrivateBaseTest has @Test void packagePrivateTest() (package-private).
• example.child.CrossPackageSameSignatureTest extends PackagePrivateBaseTest and declares an unannotated void packagePrivateTest().

Under Java's override rules (JLS §8.4.8.1), a method in a different package does not override a package-private method in the superclass (childMethod.overrides(parentMethod) returns false). Because it is not an override, JUnit 5 still discovers and runs PackagePrivateBaseTest#packagePrivateTest() when executing CrossPackageSameSignatureTest.

However, isInheritedBy() rejects all package-private methods across packages, causing CrossPackageSameSignatureTest to discover 0 tests and breaking standard JUnit 5 cross-package inheritance. We only need to skip private methods (Modifier.isPrivate(methodBinding.getModifiers())).

2. Problem: Stopping at the first overrides() match in moreSpecificMethods + keeping inheritedTestSignatures

In addInheritedTestMethods (lines 591–597):

IMethodBinding overridingMethod = null;
for (final IMethodBinding moreSpecificMethod : moreSpecificMethods) {
    if (moreSpecificMethod.overrides(methodBinding)) {
        overridingMethod = moreSpecificMethod;
        break;
    }
}

Consider the 3-level JUnit 4 hierarchy JUnit4BaseTest (@Test) -> JUnit4MiddleTest (@Test) -> JUnit4ChildTest (unannotated):

• When checking JUnit4BaseTest#inherited(), moreSpecificMethods contains [JUnit4ChildTest#inherited(), JUnit4MiddleTest#inherited()].
• Because the loop breaks on the first match, overridingMethod is set to JUnit4ChildTest#inherited() (which has no @Test annotation) and ignores JUnit4MiddleTest#inherited() (which does have @Test and was already added!).
• That is why removing inheritedTestSignatures would have caused duplicate items with the current for ... break loop, while keeping inheritedTestSignatures (keyed by getMethodSignatureKey) deduplicates by erased name + parameter types without checking overrides().

---

Solution

You can remove isInheritedBy, getMethodSignatureKey, and inheritedTestSignatures completely, and use moreSpecificMethods.stream().anyMatch(...) to check whether any more-specific overriding method suppresses the ancestor method:

1. In TestSearchUtils.java, replace addInheritedTestMethods (and delete isInheritedBy and getMethodSignatureKey):

    private static void addInheritedTestMethods(ITypeBinding classBinding, IType childType,
            List<TestFrameworkSearcher> searchers, List<JavaTestItem> accumulator) throws JavaModelException {
        if (classBinding == null || childType == null || searchers.isEmpty()) {
            return;
        }

        final List<IMethodBinding> moreSpecificMethods = new LinkedList<>();
        Collections.addAll(moreSpecificMethods, classBinding.getDeclaredMethods());

        ITypeBinding superClass = classBinding.getSuperclass();
        while (superClass != null && !"java.lang.Object".equals(superClass.getBinaryName())) {
            for (final IMethodBinding methodBinding : superClass.getDeclaredMethods()) {
                if (Modifier.isPrivate(methodBinding.getModifiers())) {
                    continue;
                }
                for (final TestFrameworkSearcher searcher : searchers) {
                    if (!searcher.isTestMethod(methodBinding)) {
                        continue;
                    }
                    final boolean suppressed = moreSpecificMethods.stream().anyMatch(candidate ->
                            candidate.overrides(methodBinding) &&
                            (searcher.getTestKind() != TestKind.JUnit || searcher.isTestMethod(candidate)));
                    if (suppressed) {
                        break;
                    }
                    final IJavaElement element = methodBinding.getJavaElement();
                    if (element == null) {
                        break;
                    }
                    final JavaTestItem methodItem = new JavaTestItemBuilder().setJavaElement(element)
                            .setLevel(TestLevel.METHOD)
                            .setKind(searcher.getTestKind())
                            .build();
                    rebaseMethodItemToChild(methodItem, childType);
                    accumulator.add(methodItem);
                    break;
                }
            }
            Collections.addAll(moreSpecificMethods, superClass.getDeclaredMethods());
            superClass = superClass.getSuperclass();
        }
    }

2. In TestSearchUtilsTest.java, update the cross-package test expectation:

Because CrossPackageSameSignatureTest#packagePrivateTest() (in example.child) does not override PackagePrivateBaseTest#packagePrivateTest() (in example.parent), the parent's @Test method is not suppressed and should still be discovered:

    @Test
    public void testCrossPackageSameSignatureDoesNotSuppressPackagePrivateParentTest() throws Exception {
        final IProject project = importProjects("preview-junit").get(0);
        final IType childType = JavaCore.create(project)
                .findType("example.child.CrossPackageSameSignatureTest");
        assertNotNull(childType);

        final List<JavaTestItem> methods = TestSearchUtils.findDirectTestChildrenForClass(
                Arrays.asList(childType.getHandleIdentifier()), new NullProgressMonitor());
        assertEquals(1, methods.size());
        assertEquals("example.child.CrossPackageSameSignatureTest#packagePrivateTest()",
                methods.get(0).getFullName());
    }

[Vinay Kumar Maheshwaram (sudovinay01)]

Thanks for taking another look. The follow-up is addressed in the pushed update: discovery skips only private superclass methods, uses IMethodBinding.overrides() rather than signature equality, and evaluates all more-specific declarations for framework-specific suppression. Tests cover a same-signature package-private method across packages and the three-level JUnit 4 case where an annotated middle override remains discoverable despite an unannotated child override.

[Vinay Kumar Maheshwaram (sudovinay01)]

Hi Jinbo Wang (@testforstephen) Changyong Gong (@chagong) wenyt (@wenytang-ms), could a code owner please review this PR when you have a chance? I’d appreciate your feedback. Thank you!

[wenyt (wenytang-ms)]

Problem / Question

rebaseMethodItemToChild updates fullName, projectName, id, and executionClassName to point to childType, but leaves uri and range pointing to the parent declaration:

1. Parent in external JAR (parentType.getResource() == null): methodItem.getUri() remains null. On the TypeScript side (src/controller/testController.ts:278), runHandler resolves the workspace folder via workspace.getWorkspaceFolder(itemsPerProject[0].uri!), which throws when a user runs an inherited binary method directly from the Test Explorer UI.
2. Parent in a different workspace project (parent-project vs child-project): Keeping the parent's uri causes workspace.getWorkspaceFolder(item.uri) to resolve the parent's workspace folder instead of the child's.
3. Parent in workspace source: Keeping the parent's uri and range attaches every child class's inherited TestItem to the parent file's editor Gutter.

---

Solution

In rebaseMethodItemToChild, set uri to childType's resource URI and clear range to null:

    private static void rebaseMethodItemToChild(JavaTestItem methodItem, IType childType)
            throws JavaModelException {
        final String fullName = methodItem.getFullName();
        final int separator = fullName != null ? fullName.indexOf('#') : -1;
        if (separator < 0) {
            return;
        }
        final String childFullName = childType.getFullyQualifiedName() + fullName.substring(separator);
        methodItem.setFullName(childFullName);
        final String childProjectName = childType.getJavaProject().getProject().getName();
        methodItem.setProjectName(childProjectName);
        methodItem.setId(childProjectName + "@" + childFullName);
        methodItem.setExecutionClassName(childType.getFullyQualifiedName());
        if (childType.getResource() != null) {
            methodItem.setUri(JDTUtils.getFileURI(childType.getResource()));
        }
        methodItem.setRange(null);
    }

[Vinay Kumar Maheshwaram (sudovinay01)]

Thanks for catching the stale parent location. Inherited items now use the child test class resource URI when available and clear the parent method range; the parent JDT method handle remains intact for source navigation. Regression assertions cover the child URI and null range for source and binary parent inheritance.

[wenyt (wenytang-ms)]

Problem / Question

There are 3 issues in addInheritedTestMethods, isInheritedBy, and getMethodSignatureKey:

1. isInheritedBy() wrongly rejects cross-package package-private test methods in JUnit 5/6:
   While Java does not allow overriding a package-private method across packages, JUnit Jupiter (JUnit 5/6) explicitly discovers and executes package-private @Test methods declared in a superclass from a different package when running the subclass (via reflection). Only private methods (Modifier.isPrivate(methodBinding.getModifiers())) should be skipped here; JLS override visibility rules are already handled by IMethodBinding.overrides().
2. getMethodSignatureKey() and inheritedTestSignatures are redundant:
   Now that moreSpecificMethods collects all methods from subclasses up the hierarchy and uses moreSpecificMethod.overrides(methodBinding), manual string key construction via getMethodSignatureKey() and inheritedTestSignatures is unnecessary duplication.
3. moreSpecificMethods loop breaks on the first overriding method (break;):
   In a 3-level JUnit 4 hierarchy (GrandParent (@Test) -> Parent (overrides without @Test) -> Child (overrides with @Test)), when superClass reaches GrandParent, moreSpecificMethods contains both Child#test and Parent#test. Stopping at the first match (Child#test, which has @Test) causes GrandParent#test to be skipped even though Parent removed @Test and cancelled the test in JUnit 4.

---

Solution

Remove isInheritedBy(), getMethodSignatureKey(), and inheritedTestSignatures (and update testPackagePrivateTestMethodIsNotInheritedAcrossPackages in TestSearchUtilsTest.java accordingly), and simplify addInheritedTestMethods to:

    private static void addInheritedTestMethods(ITypeBinding classBinding, IType childType,
            List<TestFrameworkSearcher> searchers, List<JavaTestItem> accumulator) throws JavaModelException {
        if (classBinding == null || childType == null || searchers.isEmpty()) {
            return;
        }

        final List<IMethodBinding> moreSpecificMethods = new LinkedList<>();
        Collections.addAll(moreSpecificMethods, classBinding.getDeclaredMethods());

        ITypeBinding superClass = classBinding.getSuperclass();
        while (superClass != null && !"java.lang.Object".equals(superClass.getBinaryName())) {
            for (final IMethodBinding methodBinding : superClass.getDeclaredMethods()) {
                if (Modifier.isPrivate(methodBinding.getModifiers())) {
                    continue;
                }
                final IJavaElement element = methodBinding.getJavaElement();
                if (element == null) {
                    continue;
                }
                for (final TestFrameworkSearcher searcher : searchers) {
                    if (!searcher.isTestMethod(methodBinding)) {
                        continue;
                    }
                    final boolean overriddenBySubclass = moreSpecificMethods.stream()
                            .filter(m -> m.overrides(methodBinding))
                            .anyMatch(m -> searcher.getTestKind() != TestKind.JUnit || searcher.isTestMethod(m));
                    if (overriddenBySubclass) {
                        break;
                    }
                    final JavaTestItem methodItem = new JavaTestItemBuilder().setJavaElement(element)
                            .setLevel(TestLevel.METHOD)
                            .setKind(searcher.getTestKind())
                            .build();
                    rebaseMethodItemToChild(methodItem, childType);
                    accumulator.add(methodItem);
                    break;
                }
            }
            Collections.addAll(moreSpecificMethods, superClass.getDeclaredMethods());
            superClass = superClass.getSuperclass();
        }
    }

[Vinay Kumar Maheshwaram (sudovinay01)]

Thanks for the detailed review and suggested fix. The implementation follows that approach: it skips private methods and uses JDT override relationships instead of signature-key deduplication. I added the exact three-level JUnit 4 regression you described (annotated grandparent, unannotated middle override, annotated child override), plus an unbounded-generic regression that verifies the inherited launch selector uses java.lang.Object after erasure. The existing cross-package package-private JUnit 5 regression remains covered.

The Java reactor (mvnw clean verify -Declipse.p2.mirrors=false) and npm run build-plugin both pass. I also manually ran the inherited binary test from the child class, paused in the inherited parent method under Debug, verified this was the child test class, and continued to a passing 1/1 result.

[wenyt (wenytang-ms)]

Problem / Question

1. Duplicate code paths (if (unit == null) vs else) & unnecessary AST traversal:
   ASTParser.createBindings(new IJavaElement[] { method }, new NullProgressMonitor()) works for both source IMethods (unit != null) and binary IMethods (unit == null) in JDT. Keeping the else branch that calls TestSearchUtils.parseToAst(unit, false, ...) + ASTNodeSearchUtil.getMethodDeclarationNode + loops over SingleVariableDeclaration AST nodes is redundant and leaves two parallel paths doing the same job.
2. Missing generic type erasure in formatParameterType:
   When an inherited test method comes from a generic parent class (e.g., abstract class BaseTest<T> { @Test void base(T value) {} }), parameterType.isTypeVariable() is true. In JDT (TypeBinding.getBinaryName()), calling parameterType.getBinaryName() directly on a type variable T returns JDT's internal synthetic key (e.g., pkg.BaseTest$T) instead of the runtime erased JVM class name (java.lang.Object), causing JUnit Platform's DiscoverySelectors.selectMethod to fail with ClassNotFoundException at runtime. Calling parameterType.getErasure() first resolves T to its runtime erased type (java.lang.Object or upper bound).

---

Solution

Unify both source and binary IMethods to use parser.createBindings(new IJavaElement[] { method }, ...) and call parameterType.getErasure() in formatParameterType:

    private String resolveMethodTestName(String handleId, String executionClassName) throws CoreException {
        final IMethod method = (IMethod) JavaCore.create(handleId);
        String testName = method.getElementName();
        if ((this.args.testKind == TestKind.JUnit5 || this.args.testKind == TestKind.JUnit6) &&
                method.getParameters().length > 0) {
            final ASTParser parser = ASTParser.newParser(AST.getJLSLatest());
            parser.setProject(method.getJavaProject());
            final IBinding[] bindings = parser.createBindings(new IJavaElement[] { method },
                    new NullProgressMonitor());
            if (bindings.length != 1 || !(bindings[0] instanceof IMethodBinding)) {
                throw new CoreException(new Status(IStatus.ERROR, JUnitPlugin.PLUGIN_ID, IStatus.ERROR,
                        "Cannot resolve method binding for " + method.getElementName(), null)); //$NON-NLS-1$
            }
            testName += formatParameterTypes((IMethodBinding) bindings[0]);
        }
        final String className = StringUtils.isNotBlank(executionClassName) ? executionClassName :
                method.getDeclaringType().getFullyQualifiedName();
        return className + ':' + testName;
    }

    private String formatParameterTypes(IMethodBinding methodBinding) {
        final List<String> parameters = new LinkedList<>();
        for (final ITypeBinding parameterType : methodBinding.getParameterTypes()) {
            parameters.add(formatParameterType(parameterType));
        }
        return parameters.isEmpty() ? "" : "(" + String.join(",", parameters) + ")";
    }

    private String formatParameterType(ITypeBinding parameterType) {
        final ITypeBinding erasedType = parameterType.getErasure();
        return erasedType.isPrimitive() ? erasedType.getQualifiedName() : erasedType.getBinaryName();
    }

(You can also clean up the unused CompilationUnit, MethodDeclaration, SingleVariableDeclaration, and ASTNodeSearchUtil imports above.)

[Vinay Kumar Maheshwaram (sudovinay01)]

Thanks for the suggested simplification. The launch resolver now uses ASTParser.createBindings() for both source and binary IMethods and formats the parameter types after erasure, while retaining the child execution class. Regression tests cover generic parameter erasure and a parameterized method inherited from a binary superclass; both the individual inherited method and the complete child class passed in Test Explorer.

[Vinay Kumar Maheshwaram (sudovinay01)]

Thanks for the review, wenyt (@wenytang-ms). I have addressed the feedback and pushed the changes. Could you please re-review when you have a chance?

[wenyt (wenytang-ms)]

Thanks for addressing the previous feedback and adding the test coverage! Left two minor nit comments below.

[wenyt (wenytang-ms)]

Nit: Please update this Javadoc to match the latest implementation — uri now points to the child class and range is cleared to null (only the display label and JDT handler still point at the parent declaration).

[Vinay Kumar Maheshwaram (sudovinay01)]

Thank you for catching this. I have updated the Javadoc to reflect that the URI points to the child class and the range is cleared.

[wenyt (wenytang-ms)]

Nit: This test method uses 8-space indentation instead of 4-space indentation like the rest of the file.

[Vinay Kumar Maheshwaram (sudovinay01)]

Thank you for pointing this out. I have updated the indentation to match the surrounding tests.

[Vinay Kumar Maheshwaram (sudovinay01)]

Thank you for your review, wenyt (@wenytang-ms). I have addressed these comments and pushed the updates. Could you please review the changes again when you have a chance?