Add dolt_tests to workbench #630

eric-richardson1 · 2025-09-17T16:49:58Z

This PR adds functionality to support the dolt_tests system table and dolt_test_run() table function. This includes the new "Tests" tab, which allows you to view, edit, and run tests, as well as a test runner component that allows you to run tests and view test results before merging a branch.

tbantle22

Looking good so far, started with the graphql server code

graphql-server/src/tests/test.model.ts

graphql-server/src/tests/test.resolver.ts

graphql-server/src/queryFactory/doltgres/queries.ts

graphql-server/src/queryFactory/dolt/queries.ts

tbantle22

A few higher level comments before I get into the details

web/renderer/components/pageComponents/DatabasePage/ForTests/TestList/useTestList.ts

web/renderer/components/pageComponents/DatabasePage/ForTests/TestList/TestItemRenderer.tsx

web/renderer/components/pageComponents/DatabasePage/ForTests/TestList/index.tsx

web/renderer/components/pageComponents/DatabasePage/ForTests/TestList/useTestList.ts

web/renderer/components/pageComponents/DatabasePage/ForTests/TestGroup/index.tsx

web/renderer/components/TestResultsForMergeList/index.tsx

tbantle22

Looking much better

I know you have your blog today and most of these comments are best practice related and non blocking. If needed they can be addressed after you get this all in

The one blocking one is making sure that if runTests results in an error at any point that that error is displayed on the page

Also, have you tested this with doltgres? If it doesn't work you should hide this tab for doltgres databases

graphql-server/src/queryFactory/dolt/index.ts

graphql-server/src/queryFactory/doltgres/index.ts

graphql-server/src/tests/test.resolver.ts

web/renderer/components/pageComponents/DatabasePage/ForPulls/PullActions/Merge/index.tsx

tbantle22 · 2025-09-23T14:33:15Z

web/renderer/components/pageComponents/DatabasePage/ForTests/context/state.ts

+  expandedItems: Set<string>;
+  expandedGroups: Set<string>;
+  emptyGroups: Set<string>;
+  editingTestNames: Record<string, string>;
+  tests: Test[];
+  groupedTests: Record<string, Test[]>;
+  sortedGroupEntries: Array<[string, Test[]]>;
+  testResults: Record<
+    string,
+    { status: "passed" | "failed"; error?: string } | undefined
+  >;


Can you return state: TestState from the context so you don't need to list all these types out twice?

tbantle22 · 2025-09-23T15:10:17Z

web/renderer/components/pageComponents/DatabasePage/ForTests/TestItem/index.module.css

+  flex: 1 1 auto;
+  min-width: 0;


Should use tailwind classes for all these in this file too

tbantle22 · 2025-09-23T15:10:54Z

web/renderer/components/pageComponents/DatabasePage/ForTests/TestItem/index.module.css

+  @apply w-full !important;
+  width: 100% !important;
+  min-width: 100% !important;
+  max-width: 100% !important;


Should avoid using !important whenever possible. It gets messy quickly. There are usually other ways to make it work

tbantle22 · 2025-09-23T15:11:53Z

web/renderer/components/pageComponents/DatabasePage/ForTests/TestItem/index.tsx

+  const [showDeleteConfirm, setShowDeleteConfirm] = useState(false);
+  const [localAssertionValue, setLocalAssertionValue] = useState(
+    test.assertionValue,
+  );
+  const debounceRef = useRef<ReturnType<typeof setTimeout> | null>(null);
+
+  const updateTest = useCallback(
+    (name: string, field: keyof Test, value: string) => {
+      setState({
+        tests: tests.map((test: Test) =>
+          test.testName === name ? { ...test, [field]: value } : test,
+        ),
+        hasUnsavedChanges: true,
+      });
+    },
+    [tests, setState],
+  );
+
+  const handleDeleteTest = (testName: string) => {
+    const newExpandedItems = new Set(expandedItems);
+    newExpandedItems.delete(testName);
+    setState({
+      tests: tests.filter(test => test.testName !== testName),
+      expandedItems: newExpandedItems,
+      hasUnsavedChanges: true,
+    });
+  };
+
+  const handleTestNameEdit = (testId: string, name: string) => {
+    setState({
+      editingTestNames: {
+        ...editingTestNames,
+        [testId]: name,
+      },
+    });
+  };
+
+  const handleTestNameBlur = (testName: string) => {
+    const newName = editingTestNames[testName];
+    const test = tests.find(t => t.testName === testName);
+    if (newName.trim() && newName !== test?.testName) {
+      updateTest(testName, "testName", newName.trim());
+    }
+    const newEditingTestNames = { ...editingTestNames };
+    delete newEditingTestNames[testName];
+    setState({ editingTestNames: newEditingTestNames });
+  };
+
+  const groupOptions = sortedGroupEntries
+    .map(entry => entry[0])
+    .filter(group => group !== "");
+  const isExpanded = expandedItems.has(test.testName);
+  const editingName = editingTestNames[test.testName];
+  const testResult = testResults[test.testName];
+
+  const debouncedOnUpdateTest = (field: keyof Test, value: string) => {
+    if (debounceRef.current) {
+      clearTimeout(debounceRef.current);
+    }
+    debounceRef.current = setTimeout(() => {
+      updateTest(test.testName, field, value);
+    }, 500); // 500ms debounce
+  };
+
+  useEffect(
+    () => () => {
+      if (debounceRef.current) {
+        clearTimeout(debounceRef.current);
+      }
+    },
+    [],
+  );
+
+  useEffect(() => {
+    setLocalAssertionValue(test.assertionValue);
+  }, [test.assertionValue]);
+
+  const handleDeleteClick = (e: MouseEvent) => {
+    e.stopPropagation();
+    setShowDeleteConfirm(true);
+  };
+
+  const handleConfirmDelete = () => {
+    setShowDeleteConfirm(false);
+    handleDeleteTest(test.testName);
+  };
+
+  const handleCancelDelete = () => {
+    setShowDeleteConfirm(false);
+  };
+
+  const handleAssertionValueBlur = () => {
+    if (localAssertionValue !== test.assertionValue) {
+      updateTest(test.testName, "assertionValue", localAssertionValue);
+    }
+  };


Similar to the group item comment, this could be moved into a useEditTestItem hook in this dir

tbantle22 · 2025-09-23T15:13:06Z

web/renderer/components/pageComponents/DatabasePage/ForTests/TestItem/index.tsx

+        <div className={css.expandedContent}>
+          {testResult?.status === "failed" && testResult.error && (
+            <div className={css.errorMessage}>
+              <strong>Error:</strong> {testResult.error}


Would prefer using a span with a className here over strong

web/renderer/components/pageComponents/DatabasePage/index.tsx

…ontext/index.tsx Co-authored-by: Taylor Bantle <[email protected]>

Co-authored-by: Taylor Bantle <[email protected]>

…estGroup/index.tsx Co-authored-by: Taylor Bantle <[email protected]>

tbantle22

Some error handling related comments, but none of them are blocking

tbantle22 · 2025-09-23T18:47:24Z

web/renderer/components/pageComponents/DatabasePage/ForPulls/PullActions/Merge/index.tsx

+            <span style={{ color: "red" }}>
+              Failed to load tests: {error.message}
+            </span>


Should use ErrorMsg for styling

tbantle22 · 2025-09-23T18:48:17Z

web/renderer/components/pageComponents/DatabasePage/ForPulls/PullActions/Merge/index.tsx

+            <div style={{ color: "red", padding: "8px" }}>
+              Error running tests: {runTestError || runTestsError?.message}
+            </div>


Here too. And should generally avoid using inline styling like this and use classes instead

tbantle22 · 2025-09-23T18:49:05Z

web/renderer/components/pageComponents/DatabasePage/ForPulls/PullActions/Merge/index.tsx

      });

+      if (result.error) {
+        console.error("Error running tests:", result.error);


You don't need to console the error if you're displaying the error

tbantle22 · 2025-09-23T18:50:05Z

web/renderer/components/pageComponents/DatabasePage/ForPulls/PullActions/Merge/index.tsx

  const [testResults, setTestResults] = useState<TestResult[]>([]);
-  const [runTests] = useRunTestsLazyQuery();
-  const { data } = useTestListQuery({
+  const [runTestError, setRunTestError] = useState<string | null>(null);


useApolloError might simplify some of the type casting below

tbantle22 · 2025-09-23T18:50:32Z

web/renderer/components/pageComponents/DatabasePage/ForPulls/PullActions/Merge/index.tsx

+  if (runTestsError) {
+    console.error("Run Tests query error:", runTestsError);
+  }


Don't need this

tbantle22 · 2025-09-23T18:50:47Z

web/renderer/components/pageComponents/DatabasePage/ForTests/context/index.tsx

+  useEffect(() => {
+    if (testsError) {
+      console.error("Error loading tests:", testsError);
+    }
+  }, [testsError]);


Don't need this

tbantle22 · 2025-09-23T18:54:50Z

web/renderer/components/pageComponents/DatabasePage/ForTests/context/index.tsx

-        result.data &&
-        result.data.runTests.list.length > 0 &&
-        result.data.runTests.list[0].status === "PASS";
+        {} as Record<string, { status: "passed" | "failed"; error?: string }>,


Could make { status: "passed" | "failed"; error?: string } a named type since it's repeated a few times

tbantle22 · 2025-09-23T18:56:01Z

web/renderer/components/pageComponents/DatabasePage/ForTests/context/index.tsx

+        result.data?.runTests.list.map(test =>
+          test.status === "PASS"
+            ? {
+                status: "passed",
+              }
+            : {
+                status: "failed",
+                error: test.message,
+              },
+        );


Should this be assigned to something?

tbantle22 · 2025-09-23T19:00:43Z

web/renderer/components/pageComponents/DatabasePage/ForTests/TestList/index.tsx

-              <Button>Commit</Button>
-            </Link>
-          </div>
+        {!testsLoading && !testsError && (


Should put all of this in an Inner component and use this pattern that we use in other components:

if (loading) return <Loader /> if (error) return <ErrorMsg /> return <div>{...inner test stuff that relies on query results}</div>

There's also a QueryHandler util component that does this for you

eric-richardson1 added 7 commits September 3, 2025 11:26

initial test page display functionality

0e78a39

initial run tests functionality

6fc6c4a

run tests working

ac39cdb

Merge branch 'main' into eric/dolt-test

46abe0e

smaller fixes

73c167b

run tests before PR merge + more UX improvements

ddb6213

more ux improvements and updating PR merge page

c205b85

eric-richardson1 marked this pull request as draft September 17, 2025 16:50

eric-richardson1 added 15 commits September 17, 2025 11:28

file structure refactor and cleanup

e0b9ab3

Address UI feedback

fc5c5e7

more UI feedback

58745c4

run prettier

b8d9711

run prettier - graphql server

7932781

remove unused imports

a76ffa2

more styling improvements

3649ff6

get rid of inline styling

63a9c52

factor out statusUtils class

7d10988

prettier

e2629e1

fix linter warnings

8dfc499

UI improvements

d214da9

decrease button spacing

2c325f2

run prettier

6912709

remove debug logs

ccb1545

eric-richardson1 marked this pull request as ready for review September 18, 2025 22:38

eric-richardson1 added 2 commits September 18, 2025 16:07

use blur events for updating assertion value

4b59d4b

prettier

dfbf42e

tbantle22 reviewed Sep 19, 2025

View reviewed changes

tbantle22 reviewed Sep 22, 2025

View reviewed changes

eric-richardson1 added 2 commits September 22, 2025 13:51

fix graphql-server feedback

cc9be6c

use context instead of a massive hook, refactor some event handlers

f544e7c

eric-richardson1 added 2 commits September 22, 2025 22:21

run prettier

5c46682

fix ci errors

3b943f5

tbantle22 reviewed Sep 23, 2025

View reviewed changes

eric-richardson1 and others added 6 commits September 23, 2025 10:53

hide dolt_tests for doltgres

0034fe1

add proper error handling

067ce12

Update web/renderer/components/pageComponents/DatabasePage/ForTests/c…

99b2a5f

…ontext/index.tsx Co-authored-by: Taylor Bantle <[email protected]>

Update web/renderer/components/pageComponents/DatabasePage/index.tsx

ca6d784

Co-authored-by: Taylor Bantle <[email protected]>

Update web/renderer/components/pageComponents/DatabasePage/ForTests/T…

fc81c29

…estGroup/index.tsx Co-authored-by: Taylor Bantle <[email protected]>

fix ci

2addd3c

tbantle22 approved these changes Sep 23, 2025

View reviewed changes

remove dead code

993aba2

eric-richardson1 merged commit 8de08a5 into main Sep 23, 2025
3 checks passed

Uh oh!

Add dolt_tests to workbench #630

Add dolt_tests to workbench #630

Uh oh!

Conversation

eric-richardson1 commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tbantle22 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tbantle22 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tbantle22 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tbantle22 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eric-richardson1 commented Sep 17, 2025 •

edited

Loading