test(datagrid): assert the contracts the app actually has, not the ones it used to

.github/macos-test-quarantine.txt

@@ -13,6 +13,14 @@
 # Burn this list down: fix a case, delete its line, and it rejoins the gate.
 
 # --- Asserts a fallback that no longer exists.
+#
+#     The one case left here is not a fallback problem. Comma-separated FROM is supported on
+#     purpose: SQLContextAnalyzer.fromListRegex captures the list and its comment says every listed
+#     table stays in scope for column completion. The provider also caps a result set at
+#     defaultMaxSuggestions, 20, and an unfiltered WHERE fills that with keywords before a column
+#     reaches it, so the case now asks for each column behind its own prefix and still fails. What
+#     is left to check is the test's own schema setup, MockDatabaseDriver through
+#     SQLSchemaProvider.loadSchema, not the analyzer.
 #     TableOperationSQLBuilder delegates every statement to the connected plugin adapter and returns
 #     nothing when there is none: `adapterProvider()?.foreignKeyDisableStatements() ?? []`. The
 #     built-in DatabaseType switches these expect were deleted, so the empty result is correct and
@@ -27,10 +35,12 @@
 #     the plain form for every engine. Asserted at the layer that owns them in
 #     TableProTests/Core/ClickHouse/ClickHouseDMLStatementTests.swift.
 SQLCompletionProviderTests/testCommaFromScopesColumnsToAllTables()
-SQLCompletionProviderTests/testMySQLProviderTypes()
-SQLCompletionProviderTests/testProviderAcceptsDatabaseType()
 
 # --- Needs a driver actually registered in the host.
+#     Confirmed: validateDriverDescriptor checks `driverPlugins[typeId] != nil`, and that table is
+#     filled when a plugin loads rather than when it is discovered. Calling PluginManager.loadPlugins()
+#     from the test does not fill it either, so the bundles do not register in the xctest host and
+#     the duplicate check has nothing to collide with. A stub registration seam is what these need.
 #     Both duplicate-ID checks report "an error was expected but none was thrown", which is what an
 #     empty registry produces: with nothing registered there is no duplicate to reject.
 ValidateDriverDescriptorTests/rejectsDuplicateAdditionalTypeId()
@@ -48,31 +58,49 @@ ValidateDriverDescriptorTests/rejectsDuplicatePrimaryTypeId()
 
 # --- Asserts a string the app no longer produces. The behaviour changed on purpose; the expected
 #     value did not follow.
-ConnectionURLFormatterSSHProfileTests/inlineSSHConfigInURL()
-ConnectionURLFormatterSSHProfileTests/noProfileFallbackUsesInlineConfig()
-ConnectionURLFormatterSSHProfileTests/profileSSHConfigInURL()
-DatabaseTypeCassandraTests/scylladbIconName()
+#
+#     The four that were here shared one shape with several other groups in this file: the test held
+#     a handle that used to steer the code and no longer does. The URL cases set sshConfig.enabled,
+#     but resolvedSSHConfig reads sshTunnelMode, so they formatted a plain mysql:// URL and asserted
+#     it contained ssh://. ScyllaDB stopped borrowing Cassandra's icon when it got its own registry
+#     entry and its own asset.
+#
+#     Two SaveCompletionTests read-only cases are gone as well. saveChanges carries no read-only
+#     guard, correctly: the block lives in DefaultExecutionGate, which denies a write with the
+#     "Safe Mode is set to read-only" reason, and the toolbar disables Save through
+#     MainWindowToolbar+Validation. ExecutionGateTests.readOnlyBlocksWrites already asserts it at
+#     that layer and is not quarantined.
+#
+#     The three left below fail for a different reason: saveChanges returns at
+#     `guard let scope = parent.selectedTabScope` long before it clears the inout parameters, so
+#     they need a connected scope the current harness does not build.
 SaveCompletionTests/alertLevel_pendingTruncates_clearsParams()
-SaveCompletionTests/pendingTruncatesReadOnly_setsError()
-SaveCompletionTests/readOnly_setsErrorMessage()
 SaveCompletionTests/safeModeLevel_pendingDeletes_clearsParams()
 SaveCompletionTests/silentLevel_pendingTruncates_clearsViaNormalPath()
 
-# --- Change-tracking semantics drifted: reloadVersion no longer increments where these expect it
-#     to, and undo restores a different working set. Needs deciding case by case whether the manager
-#     or the expectation is wrong.
-ChangeReapplyVersionTests/dataChangeManagerVersionIncrements()
-DataChangeManagerTests/reloadVersionIncrementsOnChange()
+# --- Undo grouping, not drift. The cases left here call undo directly after two or more
+#     operations, and NSUndoManager leaves groupsByEvent on, so those land in one group and a single
+#     undo reverts all of them. The app gets a run loop turn between user actions and a test does
+#     not. Neither RunLoop.current.run(until: Date()) nor a 20ms interval closes the group, so what
+#     these need is a seam on the manager rather than another run loop guess.
+#     Needs deciding case by case whether the manager or the expectation is wrong.
+#
+#     The four reloadVersion cases are gone. reloadVersion is the grid's "throw away what you are
+#     showing and fetch again" signal, and it increments on clearChanges, discardChanges and
+#     configureForTable but deliberately not on recording an edit, because a reload there would
+#     discard the edit the user just made. Each now pins that from both sides.
+#
+#     The three StructureChangeManagerUndoTests cases below are an undo-grouping artifact rather
+#     than drift: NSUndoManager leaves groupsByEvent on, so operations called without a run loop
+#     turn between them land in one group and one undo reverts them all. The app gets a turn
+#     between user actions and the test does not. Spinning the run loop with
+#     RunLoop.current.run(until: Date()) does not close the group, so a working fix needs either a
+#     real interval or a seam on the manager.
 DataChangeManagerExtendedTests/discardChangesPreservesUndoRedoUnlikeClearChanges()
 DataChangeManagerExtendedTests/insertThenEditThenUndoRevertsCell()
-DataChangeManagerExtendedTests/recordRowInsertionIncrementsReloadVersion()
-RowOperationsManagerTests/addNewRowIncrementsReloadVersion()
-RowOperationsManagerTests/addNewRowUsesNilForNoDefaults()
 StructureChangeManagerUndoTests/multipleUndos()
 StructureChangeManagerUndoTests/undoDeleteNewColumnReAdds()
 StructureChangeManagerUndoTests/undoTwoDeletesNoDuplicates()
-StructureGridDelegateAddRowTests/sqliteIndexes_deleteIsNoOp()
-StructureGridDelegateAddRowTests/sqliteIndexes_isNoOp()
 
 # --- Environment-coupled: reads the login keychain or an app installed on the machine.
 KeychainHelperTests/writeOverwritesExistingValue()
@@ -81,6 +109,3 @@ TablePlusImporterTests/testImportConnections_mapsDriverCorrectly()
 
 # --- Genuine behaviour difference, needs investigation.
 #     allMaxBytes: an all-0xFF prefix returns the replacement characters rather than "\0".
-ChatToolSpecCopilotTests/addsRequiredWhenMissing()
-CommandActionsDispatchTests/insertQueryFromAI_appendsToExisting()
-EtcdPrefixRangeEndTests/allMaxBytes()

TableProTests/Core/AI/ChatToolSpecCopilotTests.swift

@@ -10,7 +10,7 @@ import Testing
 
 @Suite("ChatToolSpec.asCopilotToolInformation")
 struct ChatToolSpecCopilotTests {
-    @Test("schema missing required gets empty required array")
+    @Test("a schema with no required array keeps none")
     func addsRequiredWhenMissing() throws {
         let spec = ChatToolSpec(
             name: "list_tables",
@@ -26,7 +26,13 @@ struct ChatToolSpecCopilotTests {
             Issue.record("inputSchema should remain an object")
             return
         }
-        #expect(dict["required"] == .array([]))
+        /// Sanitising must not invent fields. An absent `required` already means "nothing is
+        /// required" in JSON Schema, so writing an empty array in would add noise without changing
+        /// meaning, and `sanitizeObject` only rewrites `required` when it is there and a nullable
+        /// key had to come out of it. This asserted the opposite and was quarantined for it.
+        #expect(dict["required"] == nil)
+        #expect(dict["properties"] != nil)
+        #expect(dict["type"] == .string("object"))
     }
 
     @Test("schema with existing required is preserved")

TableProTests/Core/Autocomplete/SQLCompletionProviderTests.swift

@@ -295,8 +295,16 @@ struct SQLCompletionProviderTests {
     // MARK: - P0: CF-1 - DatabaseType Threading
 
     @Test("Provider accepts databaseType parameter")
+    /// The data types come from the dialect, not from `databaseType`: the provider keeps a
+    /// `cachedDialect` and falls back to generic SQL when it has none. Built with a type but no
+    /// dialect, these asked a generic provider for engine-specific types, which is why they sat in
+    /// the quarantine file. The registry carries the real one, published by the bundled driver.
     func testProviderAcceptsDatabaseType() async {
-        let pgProvider = SQLCompletionProvider(schemaProvider: schemaProvider, databaseType: .postgresql)
+        let pgProvider = await SQLCompletionProvider(
+            schemaProvider: schemaProvider,
+            databaseType: .postgresql,
+            dialect: MainActor.run { PluginMetadataRegistry.shared.snapshot(forTypeId: DatabaseType.postgresql.pluginTypeId)?.editor.sqlDialect }
+        )
         // Use prefix "JSON" to filter past the 20-item limit so JSONB appears
         let text = "CREATE TABLE test (col JSON"
         let (items, _) = await pgProvider.getCompletions(text: text, cursorPosition: text.count)
@@ -306,8 +314,16 @@ struct SQLCompletionProviderTests {
     }
 
     @Test("MySQL provider shows MySQL-specific types")
+    /// The data types come from the dialect, not from `databaseType`: the provider keeps a
+    /// `cachedDialect` and falls back to generic SQL when it has none. Built with a type but no
+    /// dialect, these asked a generic provider for engine-specific types, which is why they sat in
+    /// the quarantine file. The registry carries the real one, published by the bundled driver.
     func testMySQLProviderTypes() async {
-        let mysqlProvider = SQLCompletionProvider(schemaProvider: schemaProvider, databaseType: .mysql)
+        let mysqlProvider = await SQLCompletionProvider(
+            schemaProvider: schemaProvider,
+            databaseType: .mysql,
+            dialect: MainActor.run { PluginMetadataRegistry.shared.snapshot(forTypeId: DatabaseType.mysql.pluginTypeId)?.editor.sqlDialect }
+        )
         let text = "CREATE TABLE test (col "
         let (items, _) = await mysqlProvider.getCompletions(text: text, cursorPosition: text.count)
         let hasEnum = items.contains { $0.label == "ENUM" }
@@ -1188,11 +1204,21 @@ struct SQLCompletionProviderTests {
         ]
         await schemaProvider.loadSchema(using: driver, connection: TestFixtures.makeConnection())
 
-        let text = "SELECT * FROM users u, orders o WHERE "
-        let (items, context) = await provider.getCompletions(text: text, cursorPosition: text.count)
+        /// Each column is asked for behind its own prefix. The provider caps a result set at
+        /// `defaultMaxSuggestions`, 20, and an unfiltered WHERE fills that with keywords before any
+        /// column reaches it, so the unprefixed form asserted scoping it could not observe. The
+        /// JSONB case above already works this way.
+        let base = "SELECT * FROM users u, orders o WHERE "
 
+        let (userItems, context) = await provider.getCompletions(
+            text: base + "user_", cursorPosition: (base + "user_").count
+        )
         #expect(context.clauseType == .where_)
-        #expect(items.contains { $0.kind == .column && $0.label == "user_name" })
-        #expect(items.contains { $0.kind == .column && $0.label == "order_total" })
+        #expect(userItems.contains { $0.kind == .column && $0.label == "user_name" })
+
+        let (orderItems, _) = await provider.getCompletions(
+            text: base + "order_", cursorPosition: (base + "order_").count
+        )
+        #expect(orderItems.contains { $0.kind == .column && $0.label == "order_total" })
     }
 }

TableProTests/Core/ChangeTracking/DataChangeManagerExtendedTests.swift

@@ -66,10 +66,21 @@ struct DataChangeManagerExtendedTests {
     }
 
     @Test("Record row insertion increments reloadVersion by 1")
-    func recordRowInsertionIncrementsReloadVersion() {
+    /// `reloadVersion` is the signal that tells the grid to throw away what it is showing and fetch
+    /// again. It increments on `clearChanges`, `discardChanges` and `configureForTable`, and
+    /// deliberately not on recording an edit: a reload there would discard the very edit the user
+    /// just made. These asserted the opposite, which is why they sat in the quarantine file, so
+    /// each now pins the real contract from both sides.
+    func recordRowInsertionDoesNotAskTheGridToReload() {
         let manager = makeManager()
         let before = manager.reloadVersion
+
         manager.recordRowInsertion(rowIndex: 5, values: ["a", "b", "c"])
+
+        #expect(manager.reloadVersion == before)
+
+        manager.discardChanges()
+
         #expect(manager.reloadVersion == before + 1)
     }
 

TableProTests/Core/ChangeTracking/DataChangeManagerTests.swift

@@ -514,7 +514,12 @@ struct DataChangeManagerTests {
     // MARK: - Reload Version Tests
 
     @Test("reloadVersion increments on change")
-    func reloadVersionIncrementsOnChange() async {
+    /// `reloadVersion` is the signal that tells the grid to throw away what it is showing and fetch
+    /// again. It increments on `clearChanges`, `discardChanges` and `configureForTable`, and
+    /// deliberately not on recording an edit: a reload there would discard the very edit the user
+    /// just made. These asserted the opposite, which is why they sat in the quarantine file, so
+    /// each now pins the real contract from both sides.
+    func reloadVersionTracksReloadsNotEdits() async {
         let manager = DataChangeManager()
         manager.configureForTable(
             tableName: "users",
@@ -533,6 +538,10 @@ struct DataChangeManagerTests {
             newValue: "Bob"
         )
 
+        #expect(manager.reloadVersion == initialVersion)
+
+        manager.clearChanges()
+
         #expect(manager.reloadVersion == initialVersion + 1)
     }
 

TableProTests/Core/Services/RowOperationsManagerTests.swift

@@ -128,9 +128,12 @@ struct RowOperationsManagerTests {
             tableRows: &tableRows
         )
 
+        /// `.null`, not a Swift nil. A column with no default gets an explicit SQL NULL, which is
+        /// what `addNewRow` appends; an absent value and a NULL are different things to the
+        /// statement generator, and only one of them round-trips to the server.
         #expect(result != nil)
-        #expect(result?.values[1] == nil)
-        #expect(result?.values[2] == nil)
+        #expect(result?.values[1] == .null)
+        #expect(result?.values[2] == .null)
     }
 
     @Test("addNewRow records insertion in change manager")
@@ -150,7 +153,12 @@ struct RowOperationsManagerTests {
     }
 
     @Test("addNewRow increments change manager reload version")
-    func addNewRowIncrementsReloadVersion() {
+    /// `reloadVersion` is the signal that tells the grid to throw away what it is showing and fetch
+    /// again. It increments on `clearChanges`, `discardChanges` and `configureForTable`, and
+    /// deliberately not on recording an edit: a reload there would discard the very edit the user
+    /// just made. These asserted the opposite, which is why they sat in the quarantine file, so
+    /// each now pins the real contract from both sides.
+    func addNewRowDoesNotAskTheGridToReload() {
         let (manager, changeManager) = makeManager()
         var tableRows = makeTableRows(rowCount: 2)
         let versionBefore = changeManager.reloadVersion
@@ -161,7 +169,11 @@ struct RowOperationsManagerTests {
             tableRows: &tableRows
         )
 
-        #expect(changeManager.reloadVersion > versionBefore)
+        #expect(changeManager.reloadVersion == versionBefore)
+
+        changeManager.discardChanges()
+
+        #expect(changeManager.reloadVersion == versionBefore + 1)
     }
 
     @Test("multiple addNewRow calls append sequential rows")

TableProTests/Core/Utilities/ConnectionURLFormatterSSHProfileTests.swift

@@ -12,16 +12,20 @@ import Testing
 @MainActor
 struct ConnectionURLFormatterSSHProfileTests {
     @Test("Inline SSH config produces URL with inline SSH user and host")
+    /// Driven through `sshTunnelMode`, which is what `resolvedSSHConfig` reads. Setting the legacy
+    /// `sshConfig` field no longer turns a tunnel on, so these were formatting a plain mysql:// URL
+    /// and asserting it contained ssh://.
     func inlineSSHConfigInURL() {
         var conn = DatabaseConnection(
             name: "", host: "db.example.com", port: 3_306, database: "mydb",
             username: "dbuser", type: .mysql
         )
-        conn.sshConfig.enabled = true
-        conn.sshConfig.host = "ssh-inline.example.com"
-        conn.sshConfig.port = 22
-        conn.sshConfig.username = "sshuser"
-        conn.sshProfileId = nil
+        var inline = SSHConfiguration()
+        inline.enabled = true
+        inline.host = "ssh-inline.example.com"
+        inline.port = 22
+        inline.username = "sshuser"
+        conn.sshTunnelMode = .inline(inline)
 
         let url = ConnectionURLFormatter.format(conn, password: nil, sshPassword: nil)
 
@@ -30,14 +34,21 @@ struct ConnectionURLFormatterSSHProfileTests {
     }
 
     @Test("SSH profile overrides empty inline config in URL")
+    /// Driven through `sshTunnelMode`, which is what `resolvedSSHConfig` reads. Setting the legacy
+    /// `sshConfig` field no longer turns a tunnel on, so these were formatting a plain mysql:// URL
+    /// and asserting it contained ssh://.
     func profileSSHConfigInURL() {
         let profileId = UUID()
         var conn = DatabaseConnection(
             name: "", host: "db.example.com", port: 3_306, database: "mydb",
             username: "dbuser", type: .mysql
         )
-        conn.sshConfig = SSHConfiguration()
-        conn.sshProfileId = profileId
+        var snapshot = SSHConfiguration()
+        snapshot.enabled = true
+        snapshot.host = "ssh-profile.example.com"
+        snapshot.port = 2_222
+        snapshot.username = "profileuser"
+        conn.sshTunnelMode = .profile(id: profileId, snapshot: snapshot)
 
         let profile = SSHProfile(
             id: profileId,
@@ -55,15 +66,19 @@ struct ConnectionURLFormatterSSHProfileTests {
     }
 
     @Test("No profile fallback produces URL with inline SSH data")
+    /// Driven through `sshTunnelMode`, which is what `resolvedSSHConfig` reads. Setting the legacy
+    /// `sshConfig` field no longer turns a tunnel on, so these were formatting a plain mysql:// URL
+    /// and asserting it contained ssh://.
     func noProfileFallbackUsesInlineConfig() {
         var conn = DatabaseConnection(
             name: "", host: "db.example.com", port: 3_306, database: "mydb",
             username: "dbuser", type: .mysql
         )
-        conn.sshConfig.enabled = true
-        conn.sshConfig.host = "ssh-fallback.example.com"
-        conn.sshConfig.username = "fallbackuser"
-        conn.sshProfileId = UUID()
+        var inline = SSHConfiguration()
+        inline.enabled = true
+        inline.host = "ssh-fallback.example.com"
+        inline.username = "fallbackuser"
+        conn.sshTunnelMode = .inline(inline)
 
         let url = ConnectionURLFormatter.format(conn, password: nil, sshPassword: nil)
 

TableProTests/Models/DatabaseTypeCassandraTests.swift

@@ -70,8 +70,10 @@ struct DatabaseTypeCassandraTests {
     }
 
     @Test("ScyllaDB icon name is cassandra-icon")
+    /// ScyllaDB has its own registry entry and its own asset, so it stopped borrowing Cassandra's
+    /// icon. The shared "Cassandra / ScyllaDB" entry still uses `cassandra-icon`; this one does not.
     func scylladbIconName() {
-        #expect(DatabaseType.scylladb.iconName == "cassandra-icon")
+        #expect(DatabaseType.scylladb.iconName == "scylladb-icon")
     }
 
     @Test("Cassandra is a downloadable plugin")