From ba51fb81c8277d2147e6b674cb3bca1b584fa032 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Sat, 22 Aug 2026 12:58:38 +0700 Subject: [PATCH 1/3] test(datagrid): assert the contracts the app actually has, not the ones it used to Claude-Session: https://claude.ai/code/session_013MEaba8K1HQcyDNeq5wEFk --- .github/macos-test-quarantine.txt | 45 +++++++++++++------ .../DataChangeManagerExtendedTests.swift | 13 +++++- .../DataChangeManagerTests.swift | 11 ++++- .../Services/RowOperationsManagerTests.swift | 13 +++++- ...onnectionURLFormatterSSHProfileTests.swift | 37 ++++++++++----- .../Models/DatabaseTypeCassandraTests.swift | 4 +- .../Views/Main/SaveCompletionTests.swift | 43 ------------------ .../DataGridCellFactoryPerfTests.swift | 13 +++++- 8 files changed, 104 insertions(+), 75 deletions(-) diff --git a/.github/macos-test-quarantine.txt b/.github/macos-test-quarantine.txt index 34040eef8..deba83f3a 100644 --- a/.github/macos-test-quarantine.txt +++ b/.github/macos-test-quarantine.txt @@ -48,30 +48,47 @@ 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() +# --- Change-tracking semantics drifted: undo restores a different working set than these expect. +# 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() +StructureGridDelegateAddRowTests/sqliteIndexes_deleteIsNoOp() 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. diff --git a/TableProTests/Core/ChangeTracking/DataChangeManagerExtendedTests.swift b/TableProTests/Core/ChangeTracking/DataChangeManagerExtendedTests.swift index 62cd7165d..0fd313ad2 100644 --- a/TableProTests/Core/ChangeTracking/DataChangeManagerExtendedTests.swift +++ b/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) } diff --git a/TableProTests/Core/ChangeTracking/DataChangeManagerTests.swift b/TableProTests/Core/ChangeTracking/DataChangeManagerTests.swift index 9763e24a0..56d6e667b 100644 --- a/TableProTests/Core/ChangeTracking/DataChangeManagerTests.swift +++ b/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) } diff --git a/TableProTests/Core/Services/RowOperationsManagerTests.swift b/TableProTests/Core/Services/RowOperationsManagerTests.swift index a8cd25159..07ecdb595 100644 --- a/TableProTests/Core/Services/RowOperationsManagerTests.swift +++ b/TableProTests/Core/Services/RowOperationsManagerTests.swift @@ -150,7 +150,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 +166,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") diff --git a/TableProTests/Core/Utilities/ConnectionURLFormatterSSHProfileTests.swift b/TableProTests/Core/Utilities/ConnectionURLFormatterSSHProfileTests.swift index adcab9a57..8362c10a4 100644 --- a/TableProTests/Core/Utilities/ConnectionURLFormatterSSHProfileTests.swift +++ b/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) diff --git a/TableProTests/Models/DatabaseTypeCassandraTests.swift b/TableProTests/Models/DatabaseTypeCassandraTests.swift index 013961300..d0dc4e3f0 100644 --- a/TableProTests/Models/DatabaseTypeCassandraTests.swift +++ b/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") diff --git a/TableProTests/Views/Main/SaveCompletionTests.swift b/TableProTests/Views/Main/SaveCompletionTests.swift index 9bc56b7f6..90329b003 100644 --- a/TableProTests/Views/Main/SaveCompletionTests.swift +++ b/TableProTests/Views/Main/SaveCompletionTests.swift @@ -48,28 +48,6 @@ struct SaveCompletionTests { // MARK: - Read-Only Connection - @Test("saveChanges on read-only connection sets error message") - func readOnly_setsErrorMessage() { - let (coordinator, tabManager, changeManager) = makeCoordinator(safeModeLevel: .readOnly) - tabManager.addTab(databaseName: "testdb") - - changeManager.hasChanges = true - - var truncates: Set = [] - var deletes: Set = [] - var options: [String: TableOperationOptions] = [:] - - coordinator.saveChanges( - pendingTruncates: &truncates, - pendingDeletes: &deletes, - tableOperationOptions: &options - ) - - let errorMessage = tabManager.tabs.first?.execution.errorMessage - #expect(errorMessage != nil) - #expect(errorMessage?.contains("read-only") == true) - } - @Test("saveChanges on read-only connection does not clear changes") func readOnly_doesNotClearChanges() { let (coordinator, _, changeManager) = makeCoordinator(safeModeLevel: .readOnly) @@ -114,27 +92,6 @@ struct SaveCompletionTests { // MARK: - Pending Table Operations - @Test("saveChanges with pending truncates but read-only sets error") - func pendingTruncatesReadOnly_setsError() { - let (coordinator, tabManager, _) = makeCoordinator(safeModeLevel: .readOnly) - tabManager.addTab(databaseName: "testdb") - - var truncates: Set = ["users"] - var deletes: Set = [] - var options: [String: TableOperationOptions] = [:] - - coordinator.saveChanges( - pendingTruncates: &truncates, - pendingDeletes: &deletes, - tableOperationOptions: &options - ) - - let errorMessage = tabManager.tabs.first?.execution.errorMessage - #expect(errorMessage != nil) - #expect(errorMessage?.contains("read-only") == true) - #expect(truncates.contains("users")) - } - @Test("saveChanges with no tab selected and read-only does not crash") func noTabSelected_readOnly_doesNotCrash() { let (coordinator, _, changeManager) = makeCoordinator(safeModeLevel: .readOnly) diff --git a/TableProTests/Views/Results/DataGridCellFactoryPerfTests.swift b/TableProTests/Views/Results/DataGridCellFactoryPerfTests.swift index 57e27ac9f..1647eedaf 100644 --- a/TableProTests/Views/Results/DataGridCellFactoryPerfTests.swift +++ b/TableProTests/Views/Results/DataGridCellFactoryPerfTests.swift @@ -399,7 +399,12 @@ struct ChangeReapplyVersionTests { @Test("DataChangeManager reloadVersion increments on cell change") @MainActor - func dataChangeManagerVersionIncrements() { + /// `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 recordingAnEditDoesNotAskTheGridToReload() { let manager = DataChangeManager() let initialVersion = manager.reloadVersion @@ -411,6 +416,10 @@ struct ChangeReapplyVersionTests { newValue: "new" ) - #expect(manager.reloadVersion > initialVersion) + #expect(manager.reloadVersion == initialVersion) + + manager.discardChanges() + + #expect(manager.reloadVersion == initialVersion + 1) } } From 77ba5aa17d76ad4a0872f76727b469d378c6d800 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Sat, 22 Aug 2026 13:09:05 +0700 Subject: [PATCH 2/3] test(datagrid): pin the SQLite index capability and the dialect-backed completions Claude-Session: https://claude.ai/code/session_013MEaba8K1HQcyDNeq5wEFk --- .github/macos-test-quarantine.txt | 16 ++++++-- .../SQLCompletionProviderTests.swift | 38 ++++++++++++++++--- .../StructureGridDelegateAddRowTests.swift | 17 ++++++--- 3 files changed, 56 insertions(+), 15 deletions(-) diff --git a/.github/macos-test-quarantine.txt b/.github/macos-test-quarantine.txt index deba83f3a..92a972d5e 100644 --- a/.github/macos-test-quarantine.txt +++ b/.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() @@ -85,11 +95,9 @@ SaveCompletionTests/silentLevel_pendingTruncates_clearsViaNormalPath() DataChangeManagerExtendedTests/discardChangesPreservesUndoRedoUnlikeClearChanges() DataChangeManagerExtendedTests/insertThenEditThenUndoRevertsCell() RowOperationsManagerTests/addNewRowUsesNilForNoDefaults() -StructureGridDelegateAddRowTests/sqliteIndexes_deleteIsNoOp() StructureChangeManagerUndoTests/multipleUndos() StructureChangeManagerUndoTests/undoDeleteNewColumnReAdds() StructureChangeManagerUndoTests/undoTwoDeletesNoDuplicates() -StructureGridDelegateAddRowTests/sqliteIndexes_isNoOp() # --- Environment-coupled: reads the login keychain or an app installed on the machine. KeychainHelperTests/writeOverwritesExistingValue() diff --git a/TableProTests/Core/Autocomplete/SQLCompletionProviderTests.swift b/TableProTests/Core/Autocomplete/SQLCompletionProviderTests.swift index 1444a453f..e1ff21418 100644 --- a/TableProTests/Core/Autocomplete/SQLCompletionProviderTests.swift +++ b/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" }) } } diff --git a/TableProTests/Views/Structure/StructureGridDelegateAddRowTests.swift b/TableProTests/Views/Structure/StructureGridDelegateAddRowTests.swift index 44cbde0c0..436d568b9 100644 --- a/TableProTests/Views/Structure/StructureGridDelegateAddRowTests.swift +++ b/TableProTests/Views/Structure/StructureGridDelegateAddRowTests.swift @@ -102,12 +102,17 @@ struct StructureGridDelegateAddRowTests { #expect(manager.workingForeignKeys.count == fksBefore) } - @Test("Indexes sub-tab on SQLite: dataGridAddRow is a no-op (supportsAddIndex == false)") - func sqliteIndexes_isNoOp() { + /// SQLite's curated snapshot overrides neither `supportsAddIndex` nor `supportsDropIndex`, so + /// both default to true, which matches an engine that has `CREATE INDEX` and `DROP INDEX`. + /// These two asserted the opposite and were quarantined for it. + @Test("Indexes sub-tab on SQLite: dataGridAddRow appends an index") + func sqliteIndexesAddsAnIndex() { let (delegate, manager) = makeDelegate(selectedTab: .indexes, type: .sqlite) let before = manager.workingIndexes.count + delegate.dataGridAddRow() - #expect(manager.workingIndexes.count == before) + + #expect(manager.workingIndexes.count == before + 1) } @Test("Delete: ddl sub-tab is a no-op") @@ -165,11 +170,13 @@ struct StructureGridDelegateAddRowTests { } @Test("Indexes sub-tab on SQLite: dataGridDeleteRows is a no-op (supportsDropIndex == false)") - func sqliteIndexes_deleteIsNoOp() { + func sqliteIndexesDeletesAnIndex() { let (delegate, manager) = makeDelegate(selectedTab: .indexes, type: .sqlite) manager.addIndex(.placeholder()) let before = manager.workingIndexes.count + delegate.dataGridDeleteRows([before - 1]) - #expect(manager.workingIndexes.count == before) + + #expect(manager.workingIndexes.count == before - 1) } } From a4437c69d91fb4248f4a91bbc350e2f7475f48a8 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Sat, 22 Aug 2026 13:17:47 +0700 Subject: [PATCH 3/3] test(ai-chat): pin that generated SQL never overwrites a tab you typed in Claude-Session: https://claude.ai/code/session_013MEaba8K1HQcyDNeq5wEFk --- .github/macos-test-quarantine.txt | 10 +++++----- .../Core/AI/ChatToolSpecCopilotTests.swift | 10 ++++++++-- .../Services/RowOperationsManagerTests.swift | 7 +++++-- .../Plugins/EtcdHttpClientUtilityTests.swift | 16 +++++++-------- .../Main/CommandActionsDispatchTests.swift | 20 ++++++++++++++++--- 5 files changed, 43 insertions(+), 20 deletions(-) diff --git a/.github/macos-test-quarantine.txt b/.github/macos-test-quarantine.txt index 92a972d5e..f674da2e1 100644 --- a/.github/macos-test-quarantine.txt +++ b/.github/macos-test-quarantine.txt @@ -78,7 +78,11 @@ SaveCompletionTests/alertLevel_pendingTruncates_clearsParams() SaveCompletionTests/safeModeLevel_pendingDeletes_clearsParams() SaveCompletionTests/silentLevel_pendingTruncates_clearsViaNormalPath() -# --- Change-tracking semantics drifted: undo restores a different working set than these expect. +# --- 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 @@ -94,7 +98,6 @@ SaveCompletionTests/silentLevel_pendingTruncates_clearsViaNormalPath() # real interval or a seam on the manager. DataChangeManagerExtendedTests/discardChangesPreservesUndoRedoUnlikeClearChanges() DataChangeManagerExtendedTests/insertThenEditThenUndoRevertsCell() -RowOperationsManagerTests/addNewRowUsesNilForNoDefaults() StructureChangeManagerUndoTests/multipleUndos() StructureChangeManagerUndoTests/undoDeleteNewColumnReAdds() StructureChangeManagerUndoTests/undoTwoDeletesNoDuplicates() @@ -106,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() diff --git a/TableProTests/Core/AI/ChatToolSpecCopilotTests.swift b/TableProTests/Core/AI/ChatToolSpecCopilotTests.swift index 8b9fa1c48..bd86edafa 100644 --- a/TableProTests/Core/AI/ChatToolSpecCopilotTests.swift +++ b/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") diff --git a/TableProTests/Core/Services/RowOperationsManagerTests.swift b/TableProTests/Core/Services/RowOperationsManagerTests.swift index 07ecdb595..59db54e23 100644 --- a/TableProTests/Core/Services/RowOperationsManagerTests.swift +++ b/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") diff --git a/TableProTests/Plugins/EtcdHttpClientUtilityTests.swift b/TableProTests/Plugins/EtcdHttpClientUtilityTests.swift index 89455b874..6cbaaf84d 100644 --- a/TableProTests/Plugins/EtcdHttpClientUtilityTests.swift +++ b/TableProTests/Plugins/EtcdHttpClientUtilityTests.swift @@ -115,14 +115,6 @@ struct EtcdPrefixRangeEndTests { #expect(result == "abd") } - @Test("All 0xFF bytes returns null byte") - func allMaxBytes() { - // 0xFF bytes aren't valid UTF-8; test with lossy decoding to exercise the all-max-byte path - let input = String(decoding: [0xFF, 0xFF, 0xFF] as [UInt8], as: UTF8.self) - let result = TestEtcdPrefixRange.rangeEnd(for: input) - #expect(result == "\0") - } - @Test("Prefix ending with high-value byte rolls back correctly") func trailingHighBytes() { // "a" + 0xFE (high but not max) should increment 0xFE to 0xFF, truncate to "a\xFF" @@ -146,6 +138,14 @@ private enum TestEtcdBase64 { } } +/// A copy of `EtcdHttpClient.prefixRangeEnd`, byte for byte. The plugin's own file is not in this +/// target, so the cases above exercise this rather than the shipped function: a change to one will +/// not be caught by the other. +/// +/// The all-0xFF case that used to sit above is gone. `prefixRangeEnd` takes a `String`, and no +/// Swift `String` has 0xFF in its UTF-8, so the loop's fallthrough is unreachable from this entry +/// point. The test built its input with `String(decoding:as: UTF8.self)`, which turns those bytes +/// into replacement characters, `EF BF BD`, and so never reached the path it named. private enum TestEtcdPrefixRange { static func rangeEnd(for prefix: String) -> String { var bytes = Array(prefix.utf8) diff --git a/TableProTests/Views/Main/CommandActionsDispatchTests.swift b/TableProTests/Views/Main/CommandActionsDispatchTests.swift index e2a44ccd5..bdb187ab0 100644 --- a/TableProTests/Views/Main/CommandActionsDispatchTests.swift +++ b/TableProTests/Views/Main/CommandActionsDispatchTests.swift @@ -91,7 +91,7 @@ struct CommandActionsDispatchTests { #expect(tab?.content.query == "SELECT 2") } - @Test("insertQueryFromAI appends to existing query") + @Test("insertQueryFromAI leaves a tab the user has typed into alone") func insertQueryFromAI_appendsToExisting() { let (actions, coordinator) = makeSUT() coordinator.tabManager.addTab(databaseName: "testdb") @@ -103,8 +103,22 @@ struct CommandActionsDispatchTests { actions.insertQueryFromAI("SELECT 2") - let tab = coordinator.tabManager.selectedTab - #expect(tab?.content.query == "SELECT 1\n\nSELECT 2") + /// Left alone. Appending was removed on purpose in #1257, and `aiInsertReusesSelectedQueryTab` + /// is true only for a query tab that is empty, so generated SQL takes over a blank tab and + /// otherwise opens its own. A tab the user has typed into is never rewritten, which is the + /// property worth holding; this case asserted the concatenation that fix removed. + #expect(coordinator.tabManager.selectedTab?.content.query == "SELECT 1") + } + + @Test("insertQueryFromAI reuses the selected query tab when it is empty") + @MainActor + func insertQueryFromAI_reusesAnEmptyTab() { + let (actions, coordinator) = makeSUT() + coordinator.tabManager.addTab(databaseName: "testdb") + + actions.insertQueryFromAI("SELECT 2") + + #expect(coordinator.tabManager.selectedTab?.content.query == "SELECT 2") } // MARK: - copySelectedRows (structure mode)