Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Zarr streaming e2e test #8137

Merged
merged 20 commits into from
Nov 11, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions conf/messages
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,6 @@ oidc.disabled=OIDC is disabled
oidc.configuration.invalid=OIDC configuration is invalid
oidc.authentication.failed=Failed to register / log in via Single-Sign-On (SSO with OIDC)

braintracing.new=An account on braintracing.org was created for you. You can use the same credentials as on WEBKNOSSOS to login.
braintracing.error=We could not automatically create an account for you on braintracing.org. Please do it on your own.
braintracing.exists=Great, you already have an account on braintracing.org. Please double check that you have uploaded all requested information.

dataset=Dataset
dataset.notFound=Dataset {0} does not exist or could not be accessed
dataset.notFoundConsiderLogin=Dataset {0} does not exist or could not be accessed. You may need to log in.
Expand Down
2 changes: 1 addition & 1 deletion docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ services:
-Ddatastore.redis.address=redis
-Ddatastore.watchFileSystem.enabled=false"
volumes:
- ./binaryData/Connectomics department:/home/${USER_NAME:-sbt-user}/webknossos/binaryData/Organization_X
- ./binaryData/Organization_X:/home/${USER_NAME:-sbt-user}/webknossos/binaryData/Organization_X

screenshot-tests:
image: scalableminds/puppeteer:master
Expand Down
56 changes: 51 additions & 5 deletions frontend/javascripts/test/backend-snapshot-tests/datasets.e2e.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
import _ from "lodash";
import { tokenUserA, setCurrToken, resetDatabase, writeTypeCheckingFile } from "test/e2e-setup";
import {
tokenUserA,
setCurrToken,
resetDatabase,
writeTypeCheckingFile,
replaceVolatileValues,
} from "test/e2e-setup";
import type { APIDataset } from "types/api_flow_types";
import * as api from "admin/admin_rest_api";
import test from "ava";
Expand All @@ -15,6 +21,7 @@ async function getFirstDataset(): Promise<APIDataset> {
test.before("Reset database and change token", async () => {
resetDatabase();
setCurrToken(tokenUserA);
await api.triggerDatasetCheck("http://localhost:9000");
});
Comment on lines 23 to 25
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling for dataset check trigger

While triggering the dataset check is good for test setup, it should handle potential failures gracefully.

Consider adding error handling:

test.before("Reset database and change token", async () => {
  resetDatabase();
  setCurrToken(tokenUserA);
-  await api.triggerDatasetCheck("http://localhost:9000");
+  try {
+    await api.triggerDatasetCheck("http://localhost:9000");
+  } catch (error) {
+    console.error("Failed to trigger dataset check:", error);
+    throw error;
+  }
});

Committable suggestion skipped: line range outside the PR's diff.

test.serial("getDatasets", async (t) => {
let datasets = await api.getDatasets();
Expand All @@ -29,27 +36,27 @@ test.serial("getDatasets", async (t) => {
writeTypeCheckingFile(datasets, "dataset", "APIDatasetCompact", {
isArray: true,
});
t.snapshot(datasets);
t.snapshot(replaceVolatileValues(datasets));
});
test("getActiveDatasets", async (t) => {
let datasets = await api.getActiveDatasetsOfMyOrganization();
datasets = _.sortBy(datasets, (d) => d.name);
t.snapshot(datasets);
t.snapshot(replaceVolatileValues(datasets));
});
test("getDatasetAccessList", async (t) => {
const dataset = await getFirstDataset();

const accessList = _.sortBy(await api.getDatasetAccessList(dataset), (user) => user.id);

t.snapshot(accessList);
t.snapshot(replaceVolatileValues(accessList));
});
test("updateDatasetTeams", async (t) => {
const [dataset, newTeams] = await Promise.all([getFirstDataset(), api.getEditableTeams()]);
const updatedDataset = await api.updateDatasetTeams(
dataset,
newTeams.map((team) => team.id),
);
t.snapshot(updatedDataset);
t.snapshot(replaceVolatileValues(updatedDataset));
// undo the Change
await api.updateDatasetTeams(
dataset,
Expand All @@ -62,3 +69,42 @@ test("updateDatasetTeams", async (t) => {
// await api.revokeDatasetSharingToken(dataset.name);
// t.pass();
// });

test("Zarr streaming", async (t) => {
const zattrsResp = await fetch("/data/zarr/Organization_X/test-dataset/segmentation/.zattrs", {
headers: new Headers(),
});
const zattrs = await zattrsResp.text();
t.snapshot(zattrs);

const rawDataResponse = await fetch(
"/data/zarr/Organization_X/test-dataset/segmentation/1/0.1.1.0",
{
headers: new Headers(),
},
);
const bytes = await rawDataResponse.arrayBuffer();
const base64 = btoa(String.fromCharCode(...new Uint8Array(bytes)));
t.snapshot(base64);
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling for fetch requests

While the test implementation is good, it should handle potential network failures or invalid responses.

Consider adding error handling:

 test("Zarr streaming", async (t) => {
   const zattrsResp = await fetch("/data/zarr/Organization_X/test-dataset/segmentation/.zattrs", {
     headers: new Headers(),
   });
+  t.true(zattrsResp.ok, `Failed to fetch .zattrs: ${zattrsResp.statusText}`);
   const zattrs = await zattrsResp.text();
   t.snapshot(zattrs);

   const rawDataResponse = await fetch(
     "/data/zarr/Organization_X/test-dataset/segmentation/1/0.1.1.0",
     {
       headers: new Headers(),
     },
   );
+  t.true(rawDataResponse.ok, `Failed to fetch raw data: ${rawDataResponse.statusText}`);
   const bytes = await rawDataResponse.arrayBuffer();
   const base64 = btoa(String.fromCharCode(...new Uint8Array(bytes)));
   t.snapshot(base64);
 });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
test("Zarr streaming", async (t) => {
const zattrsResp = await fetch("/data/zarr/Organization_X/test-dataset/segmentation/.zattrs", {
headers: new Headers(),
});
const zattrs = await zattrsResp.text();
t.snapshot(zattrs);
const rawDataResponse = await fetch(
"/data/zarr/Organization_X/test-dataset/segmentation/1/0.1.1.0",
{
headers: new Headers(),
},
);
const bytes = await rawDataResponse.arrayBuffer();
const base64 = btoa(String.fromCharCode(...new Uint8Array(bytes)));
t.snapshot(base64);
});
test("Zarr streaming", async (t) => {
const zattrsResp = await fetch("/data/zarr/Organization_X/test-dataset/segmentation/.zattrs", {
headers: new Headers(),
});
t.true(zattrsResp.ok, `Failed to fetch .zattrs: ${zattrsResp.statusText}`);
const zattrs = await zattrsResp.text();
t.snapshot(zattrs);
const rawDataResponse = await fetch(
"/data/zarr/Organization_X/test-dataset/segmentation/1/0.1.1.0",
{
headers: new Headers(),
},
);
t.true(rawDataResponse.ok, `Failed to fetch raw data: ${rawDataResponse.statusText}`);
const bytes = await rawDataResponse.arrayBuffer();
const base64 = btoa(String.fromCharCode(...new Uint8Array(bytes)));
t.snapshot(base64);
});


test("Zarr 3 streaming", async (t) => {
const zarrJsonResp = await fetch(
"/data/zarr3_experimental/Organization_X/test-dataset/segmentation/zarr.json",
{
headers: new Headers(),
},
);
const zarrJson = await zarrJsonResp.text();
t.snapshot(zarrJson);

const rawDataResponse = await fetch(
"/data/zarr3_experimental/Organization_X/test-dataset/segmentation/1/0.1.1.0",
{
headers: new Headers(),
},
);
const bytes = await rawDataResponse.arrayBuffer();
const base64 = btoa(String.fromCharCode(...new Uint8Array(bytes)));
t.snapshot(base64);
});
3 changes: 2 additions & 1 deletion frontend/javascripts/test/e2e-setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ const volatileKeys: Array<string | number | symbol> = [
"lastActivity",
"tracingTime",
"tracingId",
"sortingKey",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Snapshots need to be updated due to "sortingKey" changes

The codebase contains snapshot files that reference "sortingKey", specifically in datasets.e2e.js.md. Adding "sortingKey" to volatileKeys means these snapshots need to be recomputed as they will now handle this property differently during testing.

  • frontend/javascripts/test/snapshots/public-test/test-bundle/test/backend-snapshot-tests/datasets.e2e.js.md contains "sortingKey" references that will be affected
  • Other snapshot files may also need updates when tests are re-run with the new volatileKeys configuration
🔗 Analysis chain

Verify snapshot recomputation after adding "sortingKey".

The addition of "sortingKey" to volatileKeys is appropriate. As per the comment above the array, snapshots need to be recomputed when this array is modified.

Run the following script to check if there are any snapshot files that need updating:


Let me check the content of these snapshot files to see if they contain any instances of "sortingKey" that might be affected.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for snapshot files that might need updating
# Look for snapshot files in the test directory
fd -e snap -e snapshot . frontend/javascripts/test

Length of output: 1460


Script:

#!/bin/bash
# Search for "sortingKey" in snapshot files to understand the impact
rg "sortingKey" frontend/javascripts/test/snapshots/

Length of output: 193

];
export function replaceVolatileValues(obj: ArbitraryObject | null | undefined) {
if (obj == null) return obj;
Expand Down Expand Up @@ -130,7 +131,7 @@ export async function writeTypeCheckingFile(
const fullTypeAnnotation = options.isArray ? `Array<${typeString}>` : typeString;
fs.writeFileSync(
`frontend/javascripts/test/snapshots/type-check/test-type-checking-${name}.ts`,
`
`
import type { ${typeString} } from "types/api_flow_types";
const a: ${fullTypeAnnotation} = ${JSON.stringify(object)}`,
);
Expand Down
Loading