Change to createTestLogger

[MarioJGMsoft]

Description

There is a problem in our telemetry where not all of the test results are being reflected in out portal.
This PR consists in changing the getTestLogger function to the createTestLogger function which is a necessary part for the solution.

Also updates the pipelines to use PKG_SPECIFIER instead of PKG_PATH since it's better to have it consistent throughout our pipelines.

Reviewer Guidance

Please let me know if there is something else that I should be aware of.

Fixes: AB#42164

[Copilot]

Pull Request Overview

This PR updates how the test logger is injected and used in our telemetry setup and aligns pipeline variable naming.

• Switches from getTestLogger to createTestLogger in Mocha setup hooks
• Updates Mocha config to inject via FLUID_TEST_LOGGER_PKG_SPECIFIER rather than path
• Renames pipeline variables from FLUID_TEST_LOGGER_PKG_PATH to FLUID_TEST_LOGGER_PKG_SPECIFIER

Reviewed Changes

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

Show a summary per file

File	Description
tools/pipelines/test-service-clients.yml	Replaced FLUID_TEST_LOGGER_PKG_PATH with PKG_SPECIFIER
tools/pipelines/test-real-service.yml	Replaced FLUID_TEST_LOGGER_PKG_PATH with PKG_SPECIFIER
tools/pipelines/test-perf-benchmarks.yml	Replaced FLUID_TEST_LOGGER_PKG_PATH with PKG_SPECIFIER
packages/test/mocha-test-setup/src/mochaHooks.ts	Switched to async hooks and dynamic createTestLogger import
packages/test/mocha-test-setup/mocharc-common.cjs	Added logic to inject specifier-based module before setup

Comments suppressed due to low confidence (1)

tools/pipelines/test-service-clients.yml:54

• [nitpick] This environment variable declaration is repeated across multiple pipeline files; consider extracting it into a shared template or YAML anchor to reduce duplication.

        FLUID_TEST_LOGGER_PKG_SPECIFIER: '@ff-internal/aria-logger' # Contains createTestLogger impl to inject

[Copilot]

Using path.join with a module specifier may not resolve the package's entry point correctly. Consider using require.resolve(process.env.FLUID_TEST_LOGGER_PKG_SPECIFIER) or dynamic import to locate and load the module.

Suggested change

	const modulePath = path.join(moduleDir, process.env.FLUID_TEST_LOGGER_PKG_SPECIFIER);
	const modulePath = require.resolve(process.env.FLUID_TEST_LOGGER_PKG_SPECIFIER);

[alexvy86]

UPDATE: see my next comment below, this might actually be unnecessary.

@MarioJGMsoft while we know path.join() works I'd say let's try Copilot's suggestion; at a glance it does seem more robust.

[Copilot]

[nitpick] Wrap the dynamic import(...) in a try/catch block to handle cases where the module specifier is invalid or the package cannot be loaded, preventing unhandled promise rejections.

Suggested change

	const { createTestLogger } = await import(process.env.FLUID_TEST_LOGGER_PKG_SPECIFIER);
	originalLogger = createTestLogger();
	try {
	const { createTestLogger } = await import(process.env.FLUID_TEST_LOGGER_PKG_SPECIFIER);
	originalLogger = createTestLogger();
	} catch (error) {
	console.error(
	`Failed to load test logger package from specifier "${process.env.FLUID_TEST_LOGGER_PKG_SPECIFIER}". Using nullLogger as fallback.`,
	error,
	);
	originalLogger = nullLogger;
	}

[alexvy86]

This is a good idea, let's do it. Also, let's be a bit more defensive during the function call. We could have received a valid package that we could import but one that didn't actually export a createTestLogger function, so calling it without checking could result in throwing an error. A simple if (typeof createTestLogger !== "function") { throw new Error(The package does not export a createTestLogger function); } before calling it should be good enough.

[alexvy86]

I think we don't need to put the new on in requiredModulePaths because we're going to import it directly in mochaHooks.ts. It matters in the current approach because we never import/require that one, we just expect it to have been loaded so its global definitions exist. But since we'll actually import() now, all the changes to this file might be unnecessary. Let's try that locally (reverting the changes to this file), I suspect things will work as expected.

[alexvy86]

Suggested change

	// Flush the logger to ensure all events are sent before the next test runs.
	// After all tests ran, flush the logger to ensure all events are sent before the process exits.

[alexvy86]

Since we removed all places where we pass FLUID_TSTE_LOGGER_PKG_PATH, I'd say it's a good moment to remove this bit too (lines 51-55).