Fixing 2.0 Tests (Sail 2.0 Conformance)

[robmoffat]

No description provided.

[kriswest]

N.b. we're intermittently seeing issues with timeouts/races in the following tests, likely for similar reasons:

• 2.0:
  • 2.0-UCBasicUsage2,
  • 2.0-UCBasicUsage1
• 1.2:
  • AOpensBMultipleListen,
  • UCFilteredUsage3,
  • UCBasicUsage1,
  • TargetdResolve1

Only happens occasionally and only on CI (Github actions), but is confined to this set of tests (we are continuing to observe for others). The runners are virtualized and low-spec - they've never been observed to do this on a local machine. But that probably just means we are relying on winning a race.

[kriswest]

I think this needs sorting properly as the tests are likely to spring leaks in all sorts of random places without a slightly deeper fix to receiveContext (which is an example of a promise anti-pattern)

[kriswest]

this should not be necessary - the listener should be set-up before reaching this part of the test. Doing it this way is likely to be fragile.

[kriswest]

Ah I see the architectural problem that led to this. In the test definition here:

FDC3-conformance-framework/src/test/v2.0/advanced/fdc3.raiseIntent.ts

Lines 103 to 129 in 2e42c64

	const PrivateChannelsLifecycleEvents = "(2.0-PrivateChannelsLifecycleEvents) PrivateChannel lifecycle events are triggered when expected";
	it(PrivateChannelsLifecycleEvents, async () => {
	errorListener = await control.listenForError();
	let onUnsubscribeReceiver = control.receiveContext("onUnsubscribeTriggered");
	const intentResolution = await control.raiseIntent(Intent.kTestingIntent, ContextType.testContextX, {
	appId: IntentApp.IntentAppK,
	});
	control.validateIntentResolution(IntentApp.IntentAppK, intentResolution);
	let result = await control.getIntentResult(intentResolution);
	control.validateIntentResult(result, IntentResultType.PrivateChannel);
	let listener = await control.receiveContextStreamFromMockApp(<PrivateChannel>result, 1, 5);
	control.unsubscribeListener(listener);
	await onUnsubscribeReceiver; //should receive context from privChannel.onUnsubscribe in mock app
	let textContextXReceiver = control.receiveContext(ContextType.testContextX);
	control.privateChannelBroadcast(<PrivateChannel>result, ContextType.testContextX);
	await textContextXReceiver;
	let onUnsubscribeReceiver2 = control.receiveContext("onUnsubscribeTriggered");
	let onDisconnectReceiver = control.receiveContext("onDisconnectTriggered");
	let listener2 = await control.receiveContextStreamFromMockApp(<PrivateChannel>result, 6, 10);
	control.disconnectPrivateChannel(<PrivateChannel>result);
	//confirm that onUnsubscribe and onDisconnect were triggered in intent-k
	await onUnsubscribeReceiver2;
	await onDisconnectReceiver;
	control.unsubscribeListener(listener2);
	});
	});

The listeners being added via receiveContext is not awaited (because it can't be) and hence it doesn't happen without a sleep (javascript keeps running synchronously until it hits some other async call - then runs the various async calls). It can't be awaited as receiveContext is poorly designed:

FDC3-conformance-framework/src/test/v2.0/support/intent-support-2.0.ts

Lines 12 to 28 in 59884d2

	async receiveContext(contextType: string, waitTime?: number): Promise<AppControlContext> {
	let timeout;
	const appControlChannel = await getOrCreateChannel(constants.ControlChannel);
	return new Promise<Context>(async (resolve, reject) => {
	const listener = await appControlChannel.addContextListener(contextType, (context: AppControlContext) => {
	resolve(context);
	clearTimeout(timeout);
	listener.unsubscribe();
	});
	//if no context received reject promise
	const { promise: sleepPromise, timeout: theTimeout } = sleep(waitTime ?? constants.WaitTime);
	timeout = theTimeout;
	await sleepPromise;
	reject(new Error("No context received. Listener expected to receive context of type " + contextType + " from mock app"));
	});
	}

FDC3 API calls are async - although receiveContext awaits addContextListener you can't await receiveContext as it only resolves when the listener receives something - i.e. its got two different async events in one function. The correct solution to this would have been to have receiveContext return an object with the listener's promise embedded in it. You could then await receiveContext and be sure the listener was added and receive the object back. You can then separately await the second promise that will resolve when the listener receives data.

This needs fixing properly as it's going to be introducing races and fragility everywhere receiveContextis used. Sounds hard, but isn't really. Once understood it should only take a few mins to refactor all uses of receiveContext. The thing to remember is that you should not mix new Promise with await in the same function - it's a well known anti-pattern.

receiveContext should change to:

async receiveContext(contextType: string, waitTime?: number): Promise<AppControlContext> {
    let timeout;
    const appControlChannel = await getOrCreateChannel(constants.ControlChannel);
    //wrap promise so we can await this function without it having to be resolved
    return { listenerPromise: new Promise<Context>(async (resolve, reject) => {
      const listener = await appControlChannel.addContextListener(contextType, (context: AppControlContext) => {
        resolve(context);
        clearTimeout(timeout);
        listener.unsubscribe();
      });

      //if no context received reject promise
      const { promise: sleepPromise, timeout: theTimeout } = sleep(waitTime ?? constants.WaitTime);
      timeout = theTimeout;
      await sleepPromise;
      reject(new Error("No context received. Listener expected to receive context of type " + contextType + " from mock app"));
    })};
  }

Then tests using it (e.g. this one should change calls from:

      let textContextXReceiver = control.receiveContext(ContextType.testContextX);
      control.privateChannelBroadcast(<PrivateChannel>result, ContextType.testContextX);
      await textContextXReceiver;

to:

      let { listenerPromise: textContextXReceiver } = await control.receiveContext(ContextType.testContextX);
      control.privateChannelBroadcast(<PrivateChannel>result, ContextType.testContextX);
      await textContextXReceiver;

Then no fragile waits are needed.

If you need a temporary fix, move the wait from here into the test before the privateChannelBroadcast... but I'd recommend the quick refactor as it won't take long and this is a horrible example of an antipattern.