Adds IFluidContainer end to end tests using tinylicious #21092

seanimam · 2024-05-15T17:05:58Z

Description

Adds new IFluidContainer end to end tests using Tinylicious
Edits original unit tests to remove mock implementation of AppInsightsTelemetryConsumer in favor of using the actual one

Josmithr · 2024-05-15T17:29:24Z

packages/framework/client-logger/fluid-telemetry/package.json

@@ -77,10 +77,14 @@
 "format:prettier": "prettier --write . --cache --ignore-path ../../../../.prettierignore",
 "lint": "fluid-build . --task lint",
 "lint:fix": "fluid-build . --task eslint:fix --task format",
+ "start:tinylicious:test": "tinylicious > tinylicious.log 2>&1",
 "test": "npm run test:mocha",
 "test:mocha": "npm run test:mocha:esm && echo skipping cjs to avoid overhead - npm run test:mocha:cjs",


As things are configured now, if a developer runs npm run test (or test:mocha, etc.) the tests will fail due to tinylicious not running, right? If so, we may want to reconfigure the scripts to ensure they are all runnable.

I also don't think we want test:mocha (etc.), as it will result in the tests being run as a part of the global test:mocha run, which won't be correctly set up for tinylicious to run.

I would recommend removing the mocha script variants, and update test to call test:realsvc

That said, there might also be a case for having 2 separate suites of tests here:

The end-to-end tests that run with tinylicious via test:realsvc

The simple unit tests that run via test:mocha

The main benefit being that it is still possible to write simple unit tests that don't rely (and can't rely) on tinylicious.

Then we would just want to update test to call both of them for convenience.

I also like the idea of two separate suites/kinds of tests. Keep the actual unit tests running under test:mocha so CI only runs those, and test can kick off both those and the realsvc ones. Although if we want the realsvc ones to run in CI, we'd need a test:realsvc:tinylicious:report script... which currently nothing has except the e2e tests package, so I'm hesitant to add it here. I'd be ok just triggering them locally through test for now.

How to split the two kinds of tests... I might go with *.spec.ts for the unit ones, and maybe .spec.realsvc.ts for the other ones, and pass the appropriate filter to mocha in each of the scripts? Although that's not something we do in other places as far as I know, so maybe split them into two subfolders of src/test/ with *.spec.ts suffix in both cases?

Updated with split tests.
test --> spin up tinylicious and run all unit and e2e tests
test:mocha --> run unit tests only
test:realsvc --> spin up tinylicious and run only e2e tests

packages/framework/client-logger/fluid-telemetry/src/test/containerTelemetryEndToEnd.spec.ts

…s to await a promise rather than cycle a while loop

packages/framework/client-logger/fluid-telemetry/src/test/containerTelemetryEndToEnd.spec.ts

…y consumer

…to fluidTelemetryEndToEndTests

alexvy86

Left a few comments to simplify the tests, specifically on one but they can be applied to all of them.

.../framework/client-logger/fluid-telemetry/src/test/containerTelemetryEndToEnd.spec.realsvc.ts

alexvy86 · 2024-05-21T23:15:08Z

.../framework/client-logger/fluid-telemetry/src/test/containerTelemetryEndToEnd.spec.realsvc.ts

+ const actualConsumedConnectedTelemetry: ContainerConnectedTelemetry[] =
+ telemetryConsumerConsumeSpy
+ .getCalls()
+ .map((spyCall) => spyCall.args[0] as IFluidTelemetry)
+ .filter(
+ (telemetry) =>
+ telemetry.eventName === ContainerTelemetryEventNames.CONNECTED,
+ ) as ContainerConnectedTelemetry[];
+
+ if (actualConsumedConnectedTelemetry.length === 0) {
+ console.log("Failed to find expected telemetry");
+ reject(
+ new Error(
+ "Expected TestTelemetryConsumer.consume() to be called alteast once with expected container telemetry but was not.",
+ ),
+ );
+ }


We should expect a single stable telemetry event for connected and nothing else, right? Would it be a simpler and more robust check to aggressively assert the calls array should have exactly 1 item, and that (calls[0] as IFluidTelemetry).eventName === ContainerTelemetryEventNames.CONNECTED?

My thoughts here are that over time as the framework changes new container events may be emitted during the connected process and this test just wants to see that the connected event fired rather than it being the only event that fired.

The container could start emitting more events but until we update fluid-telemetry to leverage them, what this test should see remains the same, right? If we start emitting stable telemetry for the new events, then we might need to make updates here but at that point it seems potentially expected, and IMO preferrable. Right now this feels to me like a somewhat vague test... for example it would not tell us if the container for some reason connected, disconnected, and connected again, which it shouldn't be doing based on the test set up.

Good points. I forgot we're filtering untracked event types.
Updated in commit 9b0f166

alexvy86 · 2024-05-21T23:16:53Z

.../framework/client-logger/fluid-telemetry/src/test/containerTelemetryEndToEnd.spec.realsvc.ts

+ try {
+ expect(expectedContainerTelemetry).to.deep.equal(actualContainerTelemetry);
+ // We won't know what the container containerInstanceId will be but we can still check that it is defined.
+ expect(actualContainerTelemetry.containerInstanceId)
+ .to.be.a("string")
+ .with.length.above(0);
+ // This will enable the test to finally complete.
+ resolve();
+ } catch (error) {
+ reject(error); // Reject the promise with the assertion error
+ }


If we move this out of the event listener as described above, we don't need the try/catch anymore.

Updated in commit 9b0f166

…vents in a specific position from telemetry consumer spy calls

…to fluidTelemetryEndToEndTests

Josmithr · 2024-06-05T18:22:45Z

packages/framework/client-logger/fluid-telemetry/package.json

- "test:mocha:cjs": "mocha --recursive \"dist/test/**/*.spec.*js\" --exit",
- "test:mocha:esm": "mocha --recursive \"lib/test/**/*.spec.*js\" --exit",
+ "start:tinylicious:test": "tinylicious > tinylicious.log 2>&1",
+ "test": "start-server-and-test start:tinylicious:test 7070 test:mocha:all",


Nit: this works, and might be more performant, but it diverges pattern-wise from how CI runs the tests (as separate steps). It might be helpful to be consistent with CI and just run the standard mocha tests and the real service ones separately via concurrently npm:test:mocha:unit npm:test:mocha:end-to-end.

Josmithr · 2024-06-05T18:23:23Z

packages/framework/client-logger/fluid-telemetry/package.json

@@ -94,10 +104,13 @@
 },
 "devDependencies": {
 "@arethetypeswrong/cli": "^0.15.2",
+ "@fluid-experimental/tree": "workspace:~",


This is the old shared-tree, not the new one. I'm assuming we want to use the new one here?

Josmithr · 2024-06-05T18:23:59Z

.../framework/client-logger/fluid-telemetry/src/test/containerTelemetryEndToEnd.spec.realsvc.ts

+// This test suite creates an actual IFluidContainer and confirms events are fired with the expected names during the expected events
+
+describe("container telemetry E2E", () => {
+ let schema;


We should give this a type annotation.

Josmithr · 2024-06-05T18:27:34Z

.../framework/client-logger/fluid-telemetry/src/test/containerTelemetryEndToEnd.spec.realsvc.ts

+
+ // We don't know exactly when the given container events that we're looking for will fire so we have to
+ // wrap the container.on(...) event handler within a promise that will be awaited at the end of the test.
+ const { actualContainerTelemetry, expectedContainerTelemetry } = await timeoutPromise<{


I didn't know we had this utility. Good to know!

Josmithr · 2024-06-05T19:03:08Z

.../framework/client-logger/fluid-telemetry/src/test/containerTelemetryEndToEnd.spec.realsvc.ts

+ expectedContainerTelemetry: {
+ eventName: ContainerTelemetryEventNames.CONNECTED,
+ containerId,
+ containerInstanceId: containerTelemetryFromSpy.containerInstanceId,


Can we not get the instance ID directly from our container variable? If we did, we could declare the entire "expected" object outside of the promise. But I may be missing something.

I'm remembering that we don't expose the instance ID on IFluidContainer. That's a bummer. It doesn't seem quite right to use the "actual" value for it here in the "expected" object, but the alternative would be to drill into private members of the container, which likely isn't better.

Probably worth a brief comment explaining this. Alternatively, we could omit the instance IDs from the test comparisons. Either way seems reasonable to me.

Josmithr · 2024-06-05T19:03:44Z

.../framework/client-logger/fluid-telemetry/src/test/containerTelemetryEndToEnd.spec.realsvc.ts

+ if (actualConsumedDisposedTelemetry.length === 0) {
+ reject(
+ new Error(
+ "Expected TestTelemetryConsumer.consume() to be called alteast once with expected container telemetry but was not.",


Suggested change

"Expected TestTelemetryConsumer.consume() to be called alteast once with expected container telemetry but was not.",

"Expected TestTelemetryConsumer.consume() to be called at least once with expected container telemetry but was not.",

Josmithr

Left a few more questions / suggestions. But all-in-all it looks good to me!

Adds IFluidContainer end to end tests using tinylicious

876d44f

seanimam requested a review from a team May 15, 2024 17:05

github-actions bot added area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct dependencies Pull requests that update a dependency file base: main PRs targeted against main branch labels May 15, 2024

Josmithr reviewed May 15, 2024

View reviewed changes

packages/framework/client-logger/fluid-telemetry/src/test/containerTelemetryEndToEnd.spec.ts Outdated Show resolved Hide resolved

updates base tests script to instantiate tinylicious and updates test…

b1e21e8

…s to await a promise rather than cycle a while loop

alexvy86 reviewed May 15, 2024

View reviewed changes

packages/framework/client-logger/fluid-telemetry/src/test/containerTelemetryEndToEnd.spec.ts Outdated Show resolved Hide resolved

seanimam added 3 commits May 21, 2024 16:11

updates testing scripts to differentiate unit & end to end tests

ed90e20

refactors end to end tests to reduce scope to using a simple telemetr…

e7e3bd1

…y consumer

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

30f2624

…to fluidTelemetryEndToEndTests

alexvy86 reviewed May 21, 2024

View reviewed changes

seanimam added 5 commits June 4, 2024 23:13

Updates end to end test promise structure and explicitly checks for e…

9b0f166

…vents in a specific position from telemetry consumer spy calls

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

3fa19e6

…to fluidTelemetryEndToEndTests

pnpm-lock update

485ed0f

package alphabetical sorting fix

d45b6d0

version bump for tinylicious

c35e4ed

Josmithr reviewed Jun 5, 2024

View reviewed changes

Josmithr approved these changes Jun 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds IFluidContainer end to end tests using tinylicious #21092

Adds IFluidContainer end to end tests using tinylicious #21092

seanimam commented May 15, 2024 •

edited

Josmithr May 15, 2024

Josmithr May 15, 2024 •

edited

alexvy86 May 15, 2024

seanimam May 21, 2024 •

edited

alexvy86 left a comment

alexvy86 May 21, 2024

seanimam May 22, 2024

alexvy86 May 22, 2024

seanimam Jun 5, 2024

alexvy86 May 21, 2024

seanimam Jun 5, 2024

Josmithr Jun 5, 2024

Josmithr Jun 5, 2024

Josmithr Jun 5, 2024

Josmithr Jun 5, 2024

Josmithr Jun 5, 2024

Josmithr Jun 5, 2024

Josmithr Jun 5, 2024

Josmithr left a comment

	"Expected TestTelemetryConsumer.consume() to be called alteast once with expected container telemetry but was not.",
	"Expected TestTelemetryConsumer.consume() to be called at least once with expected container telemetry but was not.",

Adds IFluidContainer end to end tests using tinylicious #21092

Are you sure you want to change the base?

Adds IFluidContainer end to end tests using tinylicious #21092

Conversation

seanimam commented May 15, 2024 • edited

Description

Choose a reason for hiding this comment

Josmithr May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanimam May 21, 2024 • edited

Choose a reason for hiding this comment

alexvy86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Josmithr left a comment

Choose a reason for hiding this comment

seanimam commented May 15, 2024 •

edited

Josmithr May 15, 2024 •

edited

seanimam May 21, 2024 •

edited