Disable triggers by default and add trigger helpers #19
Conversation
WalkthroughAdds trigger management to the Testing framework: triggers are disabled by default, with helpers to enable/disable globally or per table, track state, and restore defaults. Documentation and README updated. New tests cover trigger flows using a mock subclass that records executed SQL. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer/Test
participant T as Testing instance
participant SQL as _executeSQL/_performSQLExecution
participant DB as Database
Note over T: Default on init/reset: triggers disabled
Dev->>T: enableAllTriggers()
T->>SQL: EXECUTE "SET DATABASE PARAMETER SQL TRIGGERS ENABLED TRUE"
SQL->>DB: Execute
DB-->>SQL: OK
SQL-->>T: Done
Dev->>T: enableTableTriggers("Orders")
T->>SQL: EXECUTE "ALTER TABLE \"Orders\" ENABLE TRIGGERS"
SQL->>DB: Execute
DB-->>SQL: OK
SQL-->>T: Done
Dev->>T: restoreTriggerDefaults()
T->>SQL: EXECUTE "SET DATABASE PARAMETER SQL TRIGGERS ENABLED FALSE"
SQL->>DB: Execute
DB-->>SQL: OK
sequenceDiagram
autonumber
actor Runner as Test Runner
participant T as Testing instance
participant SQL as _executeSQL/_performSQLExecution
participant DB as Database
Runner->>T: resetForNewTest()
Note over T: Clear state & ensure triggers disabled
T->>SQL: EXECUTE "SET DATABASE PARAMETER SQL TRIGGERS ENABLED FALSE"
SQL->>DB: Execute
DB-->>SQL: OK
T->>T: _resetTriggerTracking()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
README.md (1)
13-13: Nice feature call‑out; add doc link for fast discovery.Append a pointer to the new guide section so readers can jump straight to usage details.
Apply this diff:
- - **Trigger control** - Database triggers disabled by default with opt-in helpers + - **Trigger control** - Database triggers disabled by default with opt-in helpers ([details](docs/guide.md#trigger-management))docs/guide.md (1)
823-832: Add “restore defaults” behavior note.Mention that
$t.resetForNewTest()automatically reverts trigger overrides before the next test, so most suites don’t need to call$t.restoreTriggerDefaults()explicitly.testing/Project/Sources/Classes/_TestingTriggerMock.4dm (2)
5-10: Constructor looks good; ensure predictable state.Good call to initialize
executedStatements. Consider also clearing it explicitly here so each fresh mock starts empty without relying on test code to reset it.Apply this diff:
Class constructor() Super:C1705() - If (This:C1470.executedStatements=Null:C1517) - This:C1470.executedStatements:=[] - End if + This:C1470.executedStatements:=[]
12-17: Add a convenience clearer for tests.Tests frequently reassign
executedStatements:=[]. Provide a helper to avoid direct property mutation.Apply this diff:
Function _performSQLExecution($statement : Text) If (This:C1470.executedStatements=Null:C1517) This:C1470.executedStatements:=[] End if This:C1470.executedStatements.push($statement) + +Function clearExecutedStatements() + This:C1470.executedStatements:=[]testing/Project/Sources/Classes/Testing.4dm (4)
11-12: Public state surface is useful; consider read‑only accessors.Exposing
triggersGloballyEnabledandtriggersEnabledTablespublicly eases testing but also allows external mutation. Consider getters (and private backing) to prevent accidental writes outside tests.
74-85: Skip redundant work when already at defaults.If triggers are already globally disabled and there are no table overrides, short‑circuit to avoid an extra SQL round‑trip.
Apply this diff:
Function disableAllTriggers() - This:C1470._executeSQL("ALTER DATABASE DISABLE TRIGGERS") + If (Not:C34(This:C1470.triggersGloballyEnabled) & (This:C1470.triggersEnabledTables#Null:C1517) & (This:C1470.triggersEnabledTables.length=0)) + // Already at defaults; keep state consistent and return + This:C1470._resetTriggerTracking() + return + End if + This:C1470._executeSQL("ALTER DATABASE DISABLE TRIGGERS") If (This:C1470.triggersEnabledTables#Null:C1517) var $tableName : Text For each ($tableName; This:C1470.triggersEnabledTables) This:C1470._disableTriggersForTable($tableName) End for each End if This:C1470._resetTriggerTracking()
90-110: Avoid duplicate per‑table ENABLE calls.If a table is already tracked as enabled, skip re‑enabling to reduce noise and speed up tests.
Apply this diff:
Function enableTableTriggers($tableName : Text) If ($tableName=Null:C1517) return End if If ($tableName="") return End if + // Prevent duplicate enables + If (This:C1470.triggersEnabledTables#Null:C1517) + If (This:C1470.triggersEnabledTables.indexOf($tableName)>=0) + return + End if + End if var $statement : Text $statement:="ALTER TABLE "+This:C1470._quoteIdentifier($tableName)+" ENABLE TRIGGERS" This:C1470._executeSQL($statement) If (This:C1470.triggersEnabledTables=Null:C1517) This:C1470.triggersEnabledTables:=[] End if - If (This:C1470.triggersEnabledTables.indexOf($tableName)<0) - This:C1470.triggersEnabledTables.push($tableName) - End if + This:C1470.triggersEnabledTables.push($tableName)
155-172: Capture SQL failures for easier debugging.Log (or surface) errors after execution to make flaky env issues diagnosable in CI.
Apply this diff:
Function _executeSQL($statement : Text) If ($statement="") return End if var $finalStatement : Text $finalStatement:=$statement If (Length:C16($finalStatement)>0) var $lastChar : Text $lastChar:=Substring:C12($finalStatement; Length:C16($finalStatement); 1) If ($lastChar#";") $finalStatement:=$finalStatement+";" End if End if - This:C1470._performSQLExecution($finalStatement) + This:C1470._performSQLExecution($finalStatement) + // Optional: record SQL error context (non-fatal for test helpers) + var $errCode : Integer + var $errMsg : Text + SQL GET LAST ERROR($errCode; $errMsg) + If ($errCode#0) + This:C1470.log("SQL error "+String:C10($errCode)+": "+$errMsg+" while executing: "+$finalStatement) + End iftesting/Project/Sources/Classes/_TestingTest.4dm (4)
212-227: Consider asserting idempotent ENABLE behavior.If you adopt the “skip duplicate enable” change, add an assertion that the second call does not emit extra SQL.
Apply this diff (after implementing the code change):
$mock.enableTableTriggers("Orders") - $t.assert.areEqual($t; 1; $mock.triggersEnabledTables.length; "Should avoid duplicate tracking entries") + $t.assert.areEqual($t; 1; $mock.triggersEnabledTables.length; "Should avoid duplicate tracking entries") + $t.assert.areEqual($t; 1; $mock.executedStatements.length; "Should not execute duplicate ENABLE")
272-286: Also assert table tracking is cleared on restore.
restoreTriggerDefaults()should leavetriggersEnabledTablesempty.Apply this diff:
$t.assert.areEqual($t; 1; $mock.executedStatements.length; "Should call disable when restoring defaults") $t.assert.areEqual($t; "ALTER DATABASE DISABLE TRIGGERS;"; $mock.executedStatements[0]; "Should disable triggers when restoring") $t.assert.isFalse($t; $mock.triggersGloballyEnabled; "Should clear global state on restore") + $t.assert.areEqual($t; 0; $mock.triggersEnabledTables.length; "Should clear table tracking on restore")
238-252: Add a case for disabling an untracked table.Useful to guarantee no crash and correct SQL even if the table wasn’t tracked.
Proposed addition:
Function test_disable_untracked_table_is_safe($t : cs:C1710.Testing) var $mock : cs:C1710._TestingTriggerMock $mock:=cs:C1710._TestingTriggerMock.new() $mock.executedStatements:=[] $mock.disableTableTriggers("Untracked") $t.assert.areEqual($t; "ALTER TABLE \"Untracked\" DISABLE TRIGGERS;"; $mock.executedStatements[0]; "Should still emit DISABLE for untracked table")
200-211: Minor test ergonomics: prefer helper over direct property mutation.If you add
clearExecutedStatements(), use it here instead ofexecutedStatements:=[]to avoid bypassing invariants.Also applies to: 259-269, 297-301
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(1 hunks)docs/guide.md(1 hunks)testing/Project/Sources/Classes/Testing.4dm(2 hunks)testing/Project/Sources/Classes/_TestingTest.4dm(1 hunks)testing/Project/Sources/Classes/_TestingTriggerMock.4dm(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
testing/Project/Sources/Classes/**/*.4dm
📄 CodeRabbit inference engine (CLAUDE.md)
Place 4D test classes under testing/Project/Sources/Classes/ in .4dm class files
Files:
testing/Project/Sources/Classes/_TestingTriggerMock.4dmtesting/Project/Sources/Classes/_TestingTest.4dmtesting/Project/Sources/Classes/Testing.4dm
testing/Project/Sources/Classes/*Test.4dm
📄 CodeRabbit inference engine (CLAUDE.md)
testing/Project/Sources/Classes/*Test.4dm: Name 4D test classes with the suffix "Test" for auto-discovery
Define test methods with names starting with "test_" inside test classes
Use comment-based tagging in tests via lines like "// #tags: unit, integration, slow"
Opt a suite out of parallel execution with the annotation "// #parallel: false"
Automatic transactions are enabled by default; disable per test with "// #transaction: false"
When managing transactions manually, use Testing context helpers: startTransaction, inTransaction, validateTransaction, cancelTransaction, withTransaction, withTransactionValidate
testing/Project/Sources/Classes/*Test.4dm: Name test classes so their file names end with "Test.4dm" to enable auto discovery
Within test classes, name test methods with the prefix "test_" so they are executed by the runner
Use comment-based tagging with lines like "// #tags: unit, integration, slow" to categorize tests
Annotate suites that must not run in parallel with "// #parallel: false"
Disable automatic transactions for specific tests by adding "// #transaction: false" and manage transactions manually if needed
Tag tests that should not run on Linux (e.g., "no-linux") so they can be excluded with "excludeTags=no-linux" on Linux runners
Files:
testing/Project/Sources/Classes/_TestingTest.4dm
🧠 Learnings (1)
📚 Learning: 2025-09-10T17:16:04.075Z
Learnt from: CR
PR: KyleKincer/testing#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-10T17:16:04.075Z
Learning: Applies to testing/Project/Sources/Classes/*Test.4dm : Automatic transactions are enabled by default; disable per test with "// #transaction: false"
Applied to files:
testing/Project/Sources/Classes/_TestingTest.4dmtesting/Project/Sources/Classes/Testing.4dm
🔇 Additional comments (7)
testing/Project/Sources/Classes/Testing.4dm (3)
15-24: Constructor side effect: disables triggers immediately.Matches the PR goal. Please confirm that the trigger toggle affects only the current session; otherwise creating a test context during development could affect other users/sessions.
66-73: Reset order is correct.Disabling triggers before clearing state keeps invariants tight across tests.
146-154: Identifier quoting looks correct.Null→
""and doubling inner quotes matches SQL rules.testing/Project/Sources/Classes/_TestingTest.4dm (4)
191-199: Good default-state coverage.Verifies constructor disables triggers and clears global flag.
228-237: Nice escape test.Covers embedded quotes correctly.
253-271: Solid end‑to‑end for disableAll.Verifies global + per‑table behavior and state reset.
287-304: Reset coverage is accurate.Asserts both SQL and state reinitialization.
| End if | ||
|
|
||
| var $finalStatement : Text | ||
| $finalStatement:=$statement |
There was a problem hiding this comment.
Is this final statement thing achieving anything? Feels like $statement could just be modified directly trhoughout the function.
Summary
Testingcontext is created or reset and add helper methods to control trigger stateTesting
https://chatgpt.com/codex/tasks/task_e_68d14fef1e9083249bc6580a81fa7c45
Summary by CodeRabbit
New Features
Documentation
Tests