[Dev UI] Write integration tests for `add-example-dataset-dialog`

[hugomurillomtz]

Overview

An integration test would be very good to assert that an example was correctly added to one or multiple datasets, and that the datasets table correctly displays each dataset metadata. An integration test would've prevented this bug from here: #4846.

User goal(s)

Make the add example behavior less error prone.

[MichaelDoyle]

This is a great idea @hugomurillomtz. Is the bug in the store itself? If so, in the test, can we simply assert that the store has the changes?

[hugomurillomtz]

This is a great idea @hugomurillomtz. Is the bug in the store itself? If so, in the test, can we simply assert that the store has the changes?

Yeah, the bug is there. Let me see what I can do.

[hugomurillomtz]

This is a great idea @hugomurillomtz. Is the bug in the store itself? If so, in the test, can we simply assert that the store has the changes?

This is what I got from the model:

Could we mock ToolsApiService in the component test instead?
Yes, we absolutely could! To do that, we would:

1. Provide the real DatasetStore in the test module instead of the mockDatasetStore.
2. Provide a mock for ToolsApiService in the test module's providers.
3. Spy on mockToolsApiService.updateEvalDataset to assert it was called with the right arguments when the dialog button is clicked.

Should we do it?
Usually, in Angular unit testing, it's best practice to keep tests isolated (shallow testing). By mocking the store in the component test, we only test the component's logic (e.g., "Did it construct the right request objects from the form?"). Then, the store's tests verify the next link in the chain (e.g., "Did the store pass those objects to the API correctly?").

Using the real store in a component test effectively turns it into an integration test. It can make the test setup a bit more complex because you have to ensure the real store's state is initialized correctly for the component to render, but it does provide stronger confidence that the layers are wired up correctly together.

This is because, while in AddExampleDatasetDialog we call updateEvalDataset which is in datasets.store.ts,
https://github.com/FirebasePrivate/genkit-ui/blob/8545516fbcb6649d9bb97d42063778a721ba22e8/src/app/stores/datasets.store.ts#L230

this, in turn, summons updateEvalDataset from tools-api.service.ts:
https://github.com/FirebasePrivate/genkit-ui/blob/8545516fbcb6649d9bb97d42063778a721ba22e8/src/app/services/tools-api.service.ts#L304

Also, in the add-example-dataset-dialog spec file, we're indeed asserting that the store method was summoned with the right arguments:
https://github.com/FirebasePrivate/genkit-ui/blob/8545516fbcb6649d9bb97d42063778a721ba22e8/src/app/pages/evaluations/components/add-example-dataset-dialog/add-example-dataset-dialog.component.spec.ts#L222

And in the dataset-store spec file, if I go ahead and assert that the updateEvalDataset method was summoned with the right arguments, it all passes. Here's a diff for that:

diff --git a/src/app/stores/datasets.store.spec.ts b/src/app/stores/datasets.store.spec.ts
index 0231944a..0cdb5b74 100644
--- a/src/app/stores/datasets.store.spec.ts
+++ b/src/app/stores/datasets.store.spec.ts
@@ -370,7 +370,9 @@ describe('DatasetStore', () => {
         store.updateEvalDataset(UPDATE_DATASET_REQUEST);
         await firstValueFrom(store.evalDatasetMetadatas$.pipe(skip(1)));
 
-        expect(mockToolsApiService.updateEvalDataset).toHaveBeenCalled();
+        expect(mockToolsApiService.updateEvalDataset).toHaveBeenCalledWith(
+          UPDATE_DATASET_REQUEST
+        );
         const state = await firstValueFrom(
           store.updateEvalDatasetLoadingState$
         );
@@ -390,7 +392,9 @@ describe('DatasetStore', () => {
           store.updateEvalDatasetLoadingState$.pipe(skip(1))
         );
 
-        expect(mockToolsApiService.updateEvalDataset).toHaveBeenCalled();
+        expect(mockToolsApiService.updateEvalDataset).toHaveBeenCalledWith(
+          UPDATE_DATASET_REQUEST
+        );

And everything passes:

So, I'd say, an integration test would indeed be the better and more reliable solution as it should be looking at UI elements and testing the whole interaction.