Added tests for form builder and fixed some issues

zonia3000 · zonia3000 · commit 79e0ecc81612 · 2024-05-07T13:11:35.000+02:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,10 @@
 *Note: Numbers like (\#123) point to closed Pull Requests on the fractal-web repository.*
 
+# Unreleased
+
+* Improved form builder used in workflow tasks without JSON Schema and in Meta properties tab (\#481).
+* Used collapsible sections in dataset history modal (\#481).
+
 # 1.0.3
 
 * fixed issue in task version update when no arguments fix is needed (\#477).
diff --git a/playwright.config.js b/playwright.config.js
@@ -47,7 +47,7 @@ export default defineConfig({
 
 	webServer: [
 		{
-			command: './tests/start-test-server.sh 2.0.0',
+			command: './tests/start-test-server.sh 2.0.5',
 			port: 8000,
 			waitForPort: true,
 			stdout: 'pipe',
diff --git a/src/lib/components/v2/workflow/ArgumentsSchema.svelte b/src/lib/components/v2/workflow/ArgumentsSchema.svelte
@@ -69,10 +69,11 @@
 			displayStandardErrorAlert(err, 'task-args-validation-errors');
 			return;
 		}
-		if (nonParallelFormBuilderComponent && !nonParallelFormBuilderComponent.validateArguments()) {
-			return;
-		}
-		if (parallelFormBuilderComponent && !parallelFormBuilderComponent.validateArguments()) {
+		const invalidFormBuilderNonParallel =
+			nonParallelFormBuilderComponent && !nonParallelFormBuilderComponent.validateArguments();
+		const invalidFormBuilderParallel =
+			parallelFormBuilderComponent && !parallelFormBuilderComponent.validateArguments();
+		if (invalidFormBuilderNonParallel || invalidFormBuilderParallel) {
 			return;
 		}
 		const payload = {};
@@ -179,7 +180,7 @@
 <div id="workflow-arguments-schema-panel">
 	<div id="task-args-validation-errors" />
 	{#if workflowTask.task_type === 'non_parallel' || workflowTask.task_type === 'compound'}
-		{#if hasNonParallelArgs && hasParallelArgs}
+		{#if (hasNonParallelArgs && hasParallelArgs) || (workflowTask.task_type === 'compound' && !workflowTask.is_legacy_task && !workflowTask.task.args_schema_non_parallel)}
 			<h5 class="ps-2 mt-3">Initialisation Parameters</h5>
 		{/if}
 		{#if !workflowTask.is_legacy_task && workflowTask.task.args_schema_non_parallel && isSchemaValid}
@@ -202,11 +203,11 @@
 			</div>
 		{/if}
 	{/if}
-	{#if hasNonParallelArgs && hasParallelArgs}
+	{#if (hasNonParallelArgs && hasParallelArgs) || (workflowTask.task_type === 'compound' && !argsSchemaParallel && !workflowTask.is_legacy_task && !workflowTask.task.args_schema_non_parallel)}
 		<hr />
 	{/if}
 	{#if workflowTask.task_type === 'parallel' || workflowTask.task_type === 'compound'}
-		{#if hasParallelArgs && hasNonParallelArgs}
+		{#if (hasParallelArgs && hasNonParallelArgs) || (workflowTask.task_type === 'compound' && !argsSchemaParallel)}
 			<h5 class="ps-2 mt-3">Compute Parameters</h5>
 		{/if}
 		{#if argsSchemaParallel && isSchemaValid}
diff --git a/src/lib/components/v2/workflow/FormBaseEntry.svelte b/src/lib/components/v2/workflow/FormBaseEntry.svelte
@@ -26,17 +26,17 @@
 			class="form-control"
 			placeholder="Argument default value"
 			class:is-invalid={entry.error}
-			on:input={triggerChanges}
 			bind:value={entry.value}
+			on:input={triggerChanges}
 		/>
 	{:else if entry.type === 'number'}
 		<input
 			type="number"
 			class="form-control"
 			placeholder="Argument default value"
 			class:is-invalid={entry.error}
-			on:input={triggerChanges}
 			bind:value={entry.value}
+			on:input={triggerChanges}
 		/>
 	{:else if entry.type === 'boolean'}
 		<select class="form-control" bind:value={entry.value} on:change={triggerChanges}>
diff --git a/src/lib/components/v2/workflow/FormEntry.svelte b/src/lib/components/v2/workflow/FormEntry.svelte
@@ -34,7 +34,7 @@
 							<input
 								type="text"
 								class="form-control"
-								placeholder="Key"
+								placeholder="Arg name"
 								class:is-invalid={entry.error}
 								bind:value={entry.key}
 								data-bs-toggle="collapse"
diff --git a/src/lib/components/v2/workflow/InputFiltersTab.svelte b/src/lib/components/v2/workflow/InputFiltersTab.svelte
@@ -119,7 +119,7 @@
 		return form.hasUnsavedChanges();
 	}
 
-	export function discard() {
+	export function discardChanges() {
 		form.discardChanges();
 	}
 
diff --git a/src/lib/components/v2/workflow/MetaPropertiesForm.svelte b/src/lib/components/v2/workflow/MetaPropertiesForm.svelte
@@ -34,10 +34,11 @@
 	$: unsavedChanges = unsavedChangesFormBuilderParallel || unsavedChangesFormBuilderNonParallel;
 
 	export async function saveChanges() {
-		if (nonParallelFormBuilderComponent && !nonParallelFormBuilderComponent.validateArguments()) {
-			return;
-		}
-		if (parallelFormBuilderComponent && !parallelFormBuilderComponent.validateArguments()) {
+		const invalidNonParallel =
+			nonParallelFormBuilderComponent && !nonParallelFormBuilderComponent.validateArguments();
+		const invalidParallel =
+			parallelFormBuilderComponent && !parallelFormBuilderComponent.validateArguments();
+		if (invalidNonParallel || invalidParallel) {
 			return;
 		}
 		const payload = {};
@@ -79,7 +80,7 @@
 		}
 	}
 
-	function discardChanges() {
+	export function discardChanges() {
 		if (nonParallelFormBuilderComponent) {
 			nonParallelFormBuilderComponent.discardChanges(workflowTask.meta_non_parallel);
 		}
diff --git a/src/routes/v2/projects/[projectId]/workflows/[workflowId]/+page.svelte b/src/routes/v2/projects/[projectId]/workflows/[workflowId]/+page.svelte
@@ -1218,7 +1218,7 @@
 			type="button"
 			class="btn btn-warning"
 			on:click={() => {
-				inputFiltersTab?.discard();
+				inputFiltersTab?.discardChanges();
 				setSelectedWorkflowTask(preventedSelectedTaskChange);
 			}}
 			data-bs-dismiss="modal"
diff --git a/tests/v2/import_export_args.spec.js b/tests/v2/import_export_args.spec.js
@@ -84,14 +84,14 @@ test('Import/export arguments [v2]', async ({ page, workflow }) => {
 		await page.getByPlaceholder('Arg name').click();
 		await page.getByPlaceholder('Arg name').fill('key_non_parallel');
 		await page.getByPlaceholder('Argument default value').fill('value_non_parallel');
-		await page.getByLabel('Save argument').click();
+		await page.getByRole('button', { name: 'Save changes' }).click();
 
 		const { file, data } = await exportArgs(page, nonParallelTaskWithoutArgsSchema);
 		expect(data.args_non_parallel.key_non_parallel).toEqual('value_non_parallel');
 		expect(data.args_parallel).toEqual(null);
 		const newData = { args_non_parallel: { key_non_parallel: 'value_non_parallel-updated' } };
 		await importValidArgs(page, file, newData);
-		await page.getByText('value_non_parallel-updated').waitFor();
+		await page.getByText('Arguments changes saved successfully').waitFor();
 		await workflow.removeCurrentTask();
 	});
 
@@ -102,14 +102,14 @@ test('Import/export arguments [v2]', async ({ page, workflow }) => {
 		await page.getByPlaceholder('Arg name').click();
 		await page.getByPlaceholder('Arg name').fill('key_parallel');
 		await page.getByPlaceholder('Argument default value').fill('value_parallel');
-		await page.getByLabel('Save argument').click();
+		await page.getByRole('button', { name: 'Save changes' }).click();
 
 		const { file, data } = await exportArgs(page, parallelTaskWithoutArgsSchema);
 		expect(data.args_non_parallel).toEqual(null);
 		expect(data.args_parallel.key_parallel).toEqual('value_parallel');
 		const newData = { args_parallel: { key_parallel: 'value_parallel-updated' } };
 		await importValidArgs(page, file, newData);
-		await page.getByText('value_parallel-updated').waitFor();
+		await page.getByText('Arguments changes saved successfully').waitFor();
 		await workflow.removeCurrentTask();
 	});
 
@@ -119,12 +119,10 @@ test('Import/export arguments [v2]', async ({ page, workflow }) => {
 		await page.getByRole('button', { name: 'Add property' }).first().click();
 		await page.getByPlaceholder('Arg name').fill('key_non_parallel');
 		await page.getByPlaceholder('Argument default value').fill('value_non_parallel');
-		await page.getByLabel('Save argument').click();
-		await page.getByText('Arguments changes saved successfully').waitFor();
 		await page.getByRole('button', { name: 'Add property' }).nth(1).click();
-		await page.getByPlaceholder('Arg name').fill('key_parallel');
-		await page.getByPlaceholder('Argument default value').fill('value_parallel');
-		await page.getByLabel('Save argument').click();
+		await page.getByPlaceholder('Arg name').nth(1).fill('key_parallel');
+		await page.getByPlaceholder('Argument default value').nth(1).fill('value_parallel');
+		await page.getByRole('button', { name: 'Save changes' }).click();
 		await page.getByText('Arguments changes saved successfully').waitFor();
 
 		const { file, data } = await exportArgs(page, compoundTaskWithoutArgsSchema);
@@ -135,8 +133,7 @@ test('Import/export arguments [v2]', async ({ page, workflow }) => {
 			args_parallel: { key_parallel: 'value_parallel-updated' }
 		};
 		await importValidArgs(page, file, newData);
-		await page.getByText('value_non_parallel-updated').waitFor();
-		await page.getByText('value_parallel-updated').waitFor();
+		await page.getByText('Arguments changes saved successfully').waitFor();
 		await workflow.removeCurrentTask();
 	});
 
diff --git a/tests/v2/run_mock_tasks.spec.js b/tests/v2/run_mock_tasks.spec.js
@@ -305,7 +305,7 @@ test('Collect and run mock tasks [v2]', async ({ page, workflow, request }) => {
 	});
 
 	await test.step('Cleanup zarr_dir', async () => {
-		fs.rmSync(`/tmp/playwright/datasets/${datasetName2}`, { recursive: true });
+		fs.rmSync(`/tmp/playwright/datasets/${datasetName2}`, { recursive: true, force: true });
 	});
 
 	await test.step('Restart the workflow creating a new dataset', async () => {
diff --git a/tests/v2/workflow_task_meta_properties.spec.js b/tests/v2/workflow_task_meta_properties.spec.js
@@ -1,4 +1,4 @@
-import { test } from './workflow_fixture.js';
+import { expect, test } from './workflow_fixture.js';
 import { waitPageLoading } from '../utils.js';
 import { createFakeTask, deleteTask } from './task_utils.js';
 
@@ -8,7 +8,7 @@ test('Workflow task meta properties [v2]', async ({ page, workflow }) => {
 
 	let taskName;
 	await test.step('Create test tasks', async () => {
-		taskName = await createFakeTask(page, { type: 'non_parallel' });
+		taskName = await createFakeTask(page, { type: 'compound' });
 	});
 
 	await test.step('Open workflow page', async () => {
@@ -24,25 +24,81 @@ test('Workflow task meta properties [v2]', async ({ page, workflow }) => {
 		await page.getByText('Meta', { exact: true }).click();
 	});
 
-	await test.step('Add first property', async () => {
-		await page.getByRole('button', { name: 'Add property' }).click();
-		await page.getByPlaceholder('Arg name').fill('arg-k1');
-		await page.getByPlaceholder('Argument default value').fill('arg-v1');
-		await page.getByLabel('Save argument').click();
+	const tab = page.locator('#meta-tab');
+
+	await test.step('Add non parallel properties', async () => {
+		const addPropertyBtn = tab.getByRole('button', { name: 'Add property' }).nth(0);
+		await addPropertyBtn.click();
+		await tab.getByPlaceholder('Arg name').fill('k1');
+		await tab.getByPlaceholder('Argument default value').fill('v1');
+
+		await addPropertyBtn.click();
+		await tab.getByRole('textbox', { name: 'Arg name' }).nth(1).fill('k2');
+		await tab.getByRole('combobox').nth(1).selectOption('Object');
+		await tab.locator('.accordion-button').click();
+		const objectContainer = tab.locator('.accordion-body');
+
+		await objectContainer.getByRole('button', { name: 'Add property' }).click();
+		await objectContainer.getByPlaceholder('Arg name').fill('k3');
+		await objectContainer.getByRole('combobox').selectOption('Number');
+		await objectContainer.getByRole('spinbutton').fill('42');
+
+		// Invalid property (empty key)
+		await addPropertyBtn.click();
+	});
+
+	await test.step('Add parallel properties', async () => {
+		const addPropertyBtn = tab.getByRole('button', { name: 'Add property' }).nth(2);
+		await addPropertyBtn.click();
+		await tab.getByPlaceholder('Arg name').nth(4).fill('k4');
+		await tab.getByRole('combobox').nth(3).selectOption('Array');
+
+		await tab.locator('.accordion-button').nth(1).click();
+		const arrayContainer = tab.locator('.accordion-body').nth(1);
+		await arrayContainer.getByRole('button', { name: 'Add property' }).click();
+		await arrayContainer.getByRole('combobox').selectOption('Boolean');
+
+		// Invalid property (empty number)
+		await addPropertyBtn.click();
+		await arrayContainer.getByRole('combobox').nth(2).selectOption('Number');
 	});
 
-	await test.step('Add second property', async () => {
-		await page.getByRole('button', { name: 'Add property' }).click();
-		await page.getByPlaceholder('Arg name').fill('arg-k2');
-		await page.getByPlaceholder('Argument default value').fill('arg-v2');
-		await page.getByLabel('Save argument').click();
+	await test.step('Check validation', async () => {
+		await tab.getByRole('button', { name: 'Save changes' }).click();
+		await expect(tab.getByText("Property name can't be empty")).toHaveCount(1);
+		await expect(tab.getByText('Invalid number')).toHaveCount(1);
 	});
 
-	await test.step('Verify that meta contains the 2 properties', async () => {
-		await page.getByText('arg-k1').waitFor();
-		await page.getByText('arg-k2').waitFor();
-		await page.getByText('arg-v1').waitFor();
-		await page.getByText('arg-v2').waitFor();
+	await test.step('Delete invalid properties', async () => {
+		await tab.getByLabel('Remove property').nth(3).click();
+		await tab.getByLabel('Remove property').nth(5).click();
+		await expect(tab.getByText("Property name can't be empty")).toHaveCount(0);
+		await expect(tab.getByText('Invalid number')).toHaveCount(0);
+	});
+
+	await test.step('Save changes', async () => {
+		await tab.getByRole('button', { name: 'Save changes' }).click();
+		await expect(tab.getByRole('button', { name: 'Save changes' })).toBeDisabled();
+	});
+
+	await test.step('Reload the page and verify the saved properties', async () => {
+		await page.reload();
+		await workflow.selectTask(taskName);
+		await page.getByText('Meta', { exact: true }).click();
+
+		await expect(tab.getByLabel('Remove property')).toHaveCount(5);
+		await expect(tab.getByRole('textbox', { name: 'Arg name' }).first()).toHaveValue('k1');
+		await expect(tab.getByRole('textbox', { name: 'Argument default value' }).first()).toHaveValue(
+			'v1'
+		);
+		await tab.locator('.accordion-button').first().click();
+		const objectContainer = tab.locator('.accordion-body').first();
+		await expect(objectContainer.getByRole('spinbutton')).toHaveValue('42');
+		await tab.locator('.accordion-button').nth(1).click();
+		const arrayContainer = tab.locator('.accordion-body').nth(1);
+		await expect(
+			arrayContainer.getByRole('combobox').nth(0).getByRole('option', { selected: true })
+		).toHaveText('True');
 	});
 
 	await test.step('Cleanup', async () => {
diff --git a/tests/v2/workflow_task_without_schema.spec.js b/tests/v2/workflow_task_without_schema.spec.js

Original file line number	Diff line number	Diff line change
`@@ -47,7 +47,7 @@ export default defineConfig({`
`47`	`47`
`48`	`48`	`webServer: [`
`49`	`49`	`{`
`50`		`- command: './tests/start-test-server.sh 2.0.0',`
	`50`	`+ command: './tests/start-test-server.sh 2.0.5',`
`51`	`51`	`port: 8000,`
`52`	`52`	`waitForPort: true,`
`53`	`53`	`stdout: 'pipe',`
Original file line number	Diff line number	Diff line change
`@@ -119,7 +119,7 @@`
`119`	`119`	`return form.hasUnsavedChanges();`
`120`	`120`	`}`
`121`	`121`
`122`		`- export function discard() {`
	`122`	`+ export function discardChanges() {`
`123`	`123`	`form.discardChanges();`
`124`	`124`	`}`
`125`	`125`
Original file line number	Diff line number	Diff line change
`@@ -34,10 +34,11 @@`
`34`	`34`	`$: unsavedChanges = unsavedChangesFormBuilderParallel \|\| unsavedChangesFormBuilderNonParallel;`
`35`	`35`
`36`	`36`	`export async function saveChanges() {`
`37`		`- if (nonParallelFormBuilderComponent && !nonParallelFormBuilderComponent.validateArguments()) {`
`38`		`- return;`
`39`		`- }`
`40`		`- if (parallelFormBuilderComponent && !parallelFormBuilderComponent.validateArguments()) {`
	`37`	`+ const invalidNonParallel =`
	`38`	`+ nonParallelFormBuilderComponent && !nonParallelFormBuilderComponent.validateArguments();`
	`39`	`+ const invalidParallel =`
	`40`	`+ parallelFormBuilderComponent && !parallelFormBuilderComponent.validateArguments();`
	`41`	`+ if (invalidNonParallel \|\| invalidParallel) {`
`41`	`42`	`return;`
`42`	`43`	`}`
`43`	`44`	`const payload = {};`
`@@ -79,7 +80,7 @@`
`79`	`80`	`}`
`80`	`81`	`}`
`81`	`82`
`82`		`- function discardChanges() {`
	`83`	`+ export function discardChanges() {`
`83`	`84`	`if (nonParallelFormBuilderComponent) {`
`84`	`85`	`nonParallelFormBuilderComponent.discardChanges(workflowTask.meta_non_parallel);`
`85`	`86`	`}`