WIP - #3
Conversation
|
Warning Rate limit exceeded@KyleKincer has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 58 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (16)
WalkthroughAdds per-test automatic transaction management with opt-out directive parsing, new manual transaction APIs and wrappers, updates test runner to start/cancel transactions when enabled, and adds documentation plus example/debug tests exercising transaction behaviors and isolation. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as TestFunction.run()
participant TX as Transaction System
participant Test as Test Body
Runner->>Runner: _shouldUseTransactions(classCode)
alt useTransactions = true
Runner->>TX: START TRANSACTION
Runner->>Test: execute test body
Runner->>TX: CANCEL TRANSACTION (cleanup)
else
Runner->>Test: execute test body (no transaction)
end
sequenceDiagram
participant T as Testing
participant TX as Transaction System
participant Op as Operation (closure)
T->>TX: START TRANSACTION
T->>T: save and install temporary error handler
T->>Op: execute operation
T->>T: restore previous error handler
alt operation did not fail
T->>TX: VALIDATE TRANSACTION (with withTransactionValidate)
T-->>T: return success flag
else
T->>TX: CANCEL TRANSACTION
T-->>T: return False
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (7)
CLAUDE.md (2)
74-78: Grammar tweak for clarity.Minor readability improvement: consider “ensuring that” rather than a trailing colon.
-By default, each test runs in its own transaction that is automatically rolled back after completion, ensuring: +By default, each test runs in its own transaction that is automatically rolled back after completion, ensuring that:
80-91: Call out wrapper precondition to avoid nested transactions.Recommend noting that withTransaction and withTransactionValidate are intended for use when automatic transactions are disabled via “// #transaction: false” to avoid nested/ambiguous transaction scopes.
Proposed addition (just after the code block):
- Note: Use withTransaction and withTransactionValidate only when automatic transactions are disabled for the test (// #transaction: false). They are not intended to be called inside an automatically managed transaction.
testing/Project/Sources/Classes/TestFunction.4dm (1)
209-273: Make directive parsing more robust and explicit.
- Normalize directive value to tolerate casing/spacing (e.g., “False”, “ FALSE ”).
- Optional: Document any other accepted values (only “false” currently disables).
- If (Position:C15("#transaction:"; $line)>0) - var $transactionValue : Text - $transactionValue:=Substring:C12($line; Position:C15("#transaction:"; $line)+13) - $transactionValue:=Replace string:C233($transactionValue; " "; "") // Remove spaces - - // Check for explicit disable - If ($transactionValue="false") - return False - End if - break - End if + If (Position:C15("#transaction:"; $line)>0) + var $transactionValue : Text + $transactionValue:=Substring:C12($line; Position:C15("#transaction:"; $line)+13) + $transactionValue:=Replace string:C233($transactionValue; " "; "") // Remove spaces + $transactionValue:=Lowercase($transactionValue) + // Check for explicit disable + If ($transactionValue="false") + return False + End if + break + End ifNote: If you prefer to avoid Lowercase() for compatibility, we can instead compare against both “false” and “False”.
testing/Project/Sources/Classes/TransactionExampleTest.4dm (4)
3-17: Assert that auto-transaction is active during the test.Since this test is meant to demonstrate automatic transaction rollback, assert the transaction is active to catch configuration regressions early.
Apply this diff to add the assertion:
// Simulate creating test data $t.log("Creating test data within automatic transaction") + // Verify that the framework started an automatic transaction for this test + $t.assert.isTrue($t; $t.inTransaction(); "Automatic transaction is active") + // This would normally create records, but they'll be rolled back // Example: CREATE RECORD([Table1])
18-26: Also assert that transactions are disabled for this test.You already use the directive
// #transaction: false. An explicit assertion helps detect directive parsing issues and documents intent.Apply this diff:
$t.log("Running test without automatic transactions") + $t.assert.isFalse($t; $t.inTransaction(); "Automatic transactions are disabled for this test") $t.assert.isTrue($t; True; "Test runs without transaction management")
72-93: Add an assertion to validate outer transaction remains active after inner cancel.This helps verify your nested transaction semantics (e.g., reference counting vs. true nesting).
Apply this diff:
// Cancel inner transaction $t.cancelTransaction() $t.log("Cancelled inner transaction") + $t.assert.isTrue($t; $t.inTransaction(); "Outer transaction still active after cancelling inner") // Cancel outer transaction $t.cancelTransaction()
94-109: Strengthen the isolation test by exercising actual persisted data within a transaction.Currently this test doesn’t touch persistent data; it just generates a random ID. To validate isolation more effectively, consider creating a disposable record (or writing to a scratch table) and asserting visibility within the test. The framework’s automatic rollback will clean it up. Optionally, pair this with a separate test that asserts the absence of that marker outside the transaction.
If you provide a table/field to use for scratch data, I can draft a concrete test snippet that creates, verifies, and implicitly rolls back the data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (6)
CLAUDE.md(2 hunks)testing/Project/Sources/Classes/SimpleTransactionTest.4dm(1 hunks)testing/Project/Sources/Classes/TestFunction.4dm(5 hunks)testing/Project/Sources/Classes/Testing.4dm(1 hunks)testing/Project/Sources/Classes/TransactionDebugTest.4dm(1 hunks)testing/Project/Sources/Classes/TransactionExampleTest.4dm(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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" so they are auto-discovered (e.g., TaggingSystemTest.4dm).
Within test classes, name test methods with the prefix "test_" to be executed by the runner.
Use comment-based tagging in tests with the format "// #tags: ..." (e.g., // #tags: unit, integration, slow) to enable tag-based filtering.
Files:
testing/Project/Sources/Classes/SimpleTransactionTest.4dmtesting/Project/Sources/Classes/TransactionDebugTest.4dmtesting/Project/Sources/Classes/TransactionExampleTest.4dm
🪛 LanguageTool
CLAUDE.md
[grammar] ~74-~74: There might be a mistake here.
Context: ... rolled back after completion, ensuring: - Test Isolation: Tests cannot interfere...
(QB_NEW_EN)
🔇 Additional comments (7)
CLAUDE.md (2)
14-15: Nice addition: highlights the new transaction features clearly.These bullets concisely communicate the two major capabilities introduced by this PR.
66-67: Good placement of discovery guidance ahead of the new section.This sets context properly before deep-diving into transaction semantics.
testing/Project/Sources/Classes/TestFunction.4dm (2)
10-10: Property addition looks good.Naming and intent are clear; aligns with the new per-test transaction behavior.
20-20: Constructor initialization is appropriate.Deferred decision per test based on the class source is a good approach.
testing/Project/Sources/Classes/Testing.4dm (1)
51-54: LGTM: inTransaction helper.Simple and clear; aligns with usage.
testing/Project/Sources/Classes/TransactionExampleTest.4dm (2)
1-3: Good adherence to testing conventions (class and method naming, tags).
- File name ends with "Test" for auto-discovery.
- Test methods all use the "test_" prefix.
- Comment-based tags are present and follow the required format: "// #tags: ...".
60-71: Confirm semantics of$t.fail()insidewithTransaction.Calling
$t.fail()may immediately fail/abort the test depending on the framework implementation. IfwithTransactiondoesn’t intercept that and convert it into a Boolean result, this test may hard-fail instead of asserting on$success. Verify that:
$t.fail()inside the closure does not abort the test method, andwithTransaction(...)returns False after rolling back.If not, consider using an error/exception raised inside the closure that
withTransactionhandles, or provide a helper likewithTransactionExpectingFailureto clearly test rollback-on-failure without failing the test itself.Would you like me to adjust this test to simulate a handled error (e.g., closure returns False or raises a controlled error) so we can assert rollback deterministically without relying on
$t.fail()side-effects?
| Function test_disableTransaction($t : cs:C1710.Testing) | ||
| // #transaction: false | ||
| // Test disabling transactions | ||
| $t.log("Testing with transactions disabled") | ||
| $t.assert.isTrue($t; True; "Test without transactions passes") No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Assert that transactions are disabled.
Make the intent explicit by asserting the transaction state.
// #transaction: false
// Test disabling transactions
$t.log("Testing with transactions disabled")
+ $t.assert.isTrue($t; Not:C34($t.inTransaction()); "No transaction should be active when disabled")
$t.assert.isTrue($t; True; "Test without transactions passes")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Function test_disableTransaction($t : cs:C1710.Testing) | |
| // #transaction: false | |
| // Test disabling transactions | |
| $t.log("Testing with transactions disabled") | |
| $t.assert.isTrue($t; True; "Test without transactions passes") | |
| Function test_disableTransaction($t : cs:C1710.Testing) | |
| // #transaction: false | |
| // Test disabling transactions | |
| $t.log("Testing with transactions disabled") | |
| $t.assert.isTrue($t; Not:C34($t.inTransaction()); "No transaction should be active when disabled") | |
| $t.assert.isTrue($t; True; "Test without transactions passes") |
🤖 Prompt for AI Agents
In testing/Project/Sources/Classes/SimpleTransactionTest.4dm around lines 8–12,
the test logs that transactions are disabled but does not assert that state; add
an explicit assertion that transactions are disabled by calling the appropriate
test helper or runtime property (e.g. assert that the transaction-enabled
flag/method returns false) and include a clear failure message so the test fails
if transactions are not disabled.
| // Handle transaction cleanup | ||
| If ($transactionStarted) | ||
| If (This:C1470.t.failed) | ||
| // Cancel transaction if test failed | ||
| CANCEL TRANSACTION:C241 | ||
| Else | ||
| // Always cancel transaction to ensure test isolation | ||
| // Tests should not persist data changes by default | ||
| CANCEL TRANSACTION:C241 | ||
| End if | ||
| End if | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify and harden transaction cleanup.
Both branches cancel; only cancel if a transaction is active. This avoids errors if the test manually validated and ended the transaction.
-// Handle transaction cleanup
-If ($transactionStarted)
- If (This:C1470.t.failed)
- // Cancel transaction if test failed
- CANCEL TRANSACTION:C241
- Else
- // Always cancel transaction to ensure test isolation
- // Tests should not persist data changes by default
- CANCEL TRANSACTION:C241
- End if
-End if
+// Handle transaction cleanup
+If ($transactionStarted) && (In transaction:C397)
+ // Always cancel for isolation; validated/ended transactions will skip
+ CANCEL TRANSACTION:C241
+End if Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In testing/Project/Sources/Classes/TestFunction.4dm around lines 68-79, the
inner If branches both call CANCEL TRANSACTION causing redundant code and
potential errors if the test already ended the transaction; simplify by removing
the inner If/Else and call CANCEL TRANSACTION once inside the outer If
($transactionStarted) so the transaction is only cancelled when marked as
started (which preserves tests that manually closed it) — consolidate to a
single cancel statement and remove the unused failure check for cancellation.
| Function test_basicInTransaction($t : cs:C1710.Testing) | ||
| // #transaction: false | ||
| $t.log("Testing inTransaction method") | ||
|
|
||
| var $result : Boolean | ||
| $result:=$t.inTransaction() | ||
| $t.log("inTransaction result: "+String:C10($result)) | ||
| $t.assert.isTrue($t; True; "Test completed") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Assert behavior under disabled transactions.
The test currently logs but doesn’t validate behavior. Assert that inTransaction() is false when transactions are disabled for the test.
var $result : Boolean
$result:=$t.inTransaction()
$t.log("inTransaction result: "+String:C10($result))
- $t.assert.isTrue($t; True; "Test completed")
+ $t.assert.isTrue($t; Not:C34($result); "inTransaction should be False when transactions are disabled")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Function test_basicInTransaction($t : cs:C1710.Testing) | |
| // #transaction: false | |
| $t.log("Testing inTransaction method") | |
| var $result : Boolean | |
| $result:=$t.inTransaction() | |
| $t.log("inTransaction result: "+String:C10($result)) | |
| $t.assert.isTrue($t; True; "Test completed") | |
| Function test_basicInTransaction($t : cs:C1710.Testing) | |
| // #transaction: false | |
| $t.log("Testing inTransaction method") | |
| var $result : Boolean | |
| $result:=$t.inTransaction() | |
| $t.log("inTransaction result: "+String:C10($result)) | |
| $t.assert.isTrue($t; Not:C34($result); "inTransaction should be False when transactions are disabled") |
🤖 Prompt for AI Agents
In testing/Project/Sources/Classes/TransactionDebugTest.4dm around lines 3–11,
the test only logs the inTransaction() value but doesn’t assert expected
behavior when transactions are disabled; add an assertion that $result is false
(use the test framework's assertion helper, e.g., the corresponding isFalse
assertion or invert the isTrue call) with a clear message like "inTransaction
should be false when transactions are disabled" so the test fails if
transactions are incorrectly reported as active.
| Function test_basicStartTransaction($t : cs:C1710.Testing) | ||
| // #transaction: false | ||
| $t.log("Testing startTransaction method") | ||
|
|
||
| var $result : Boolean | ||
| $result:=$t.startTransaction() | ||
| $t.log("startTransaction result: "+String:C10($result)) | ||
|
|
||
| $t.cancelTransaction() | ||
| $t.assert.isTrue($t; True; "Test completed") No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen assertions around start/cancel lifecycle.
Exercise and assert the full lifecycle: start returns True, we are in a transaction afterward, cancel clears it.
var $result : Boolean
$result:=$t.startTransaction()
$t.log("startTransaction result: "+String:C10($result))
-
- $t.cancelTransaction()
- $t.assert.isTrue($t; True; "Test completed")
+ $t.assert.isTrue($t; $result; "startTransaction should return True")
+ $t.assert.isTrue($t; $t.inTransaction(); "Should be in a transaction after starting")
+ $t.cancelTransaction()
+ $t.assert.isTrue($t; Not:C34($t.inTransaction()); "Should not be in a transaction after cancel")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Function test_basicStartTransaction($t : cs:C1710.Testing) | |
| // #transaction: false | |
| $t.log("Testing startTransaction method") | |
| var $result : Boolean | |
| $result:=$t.startTransaction() | |
| $t.log("startTransaction result: "+String:C10($result)) | |
| $t.cancelTransaction() | |
| $t.assert.isTrue($t; True; "Test completed") | |
| Function test_basicStartTransaction($t : cs:C1710.Testing) | |
| // #transaction: false | |
| $t.log("Testing startTransaction method") | |
| var $result : Boolean | |
| $result:=$t.startTransaction() | |
| $t.log("startTransaction result: "+String:C10($result)) | |
| $t.assert.isTrue($t; $result; "startTransaction should return True") | |
| $t.assert.isTrue($t; $t.inTransaction(); "Should be in a transaction after starting") | |
| $t.cancelTransaction() | |
| $t.assert.isTrue($t; Not:C34($t.inTransaction()); "Should not be in a transaction after cancel") |
🤖 Prompt for AI Agents
In testing/Project/Sources/Classes/TransactionDebugTest.4dm around lines 12 to
21, strengthen the test to fully exercise the start/cancel lifecycle: assert
that $t.startTransaction() returns True, then immediately assert the transaction
is active (call the test harness method that checks transaction state, e.g.
$t.isInTransaction() or equivalent) and finally call $t.cancelTransaction() and
assert the transaction is no longer active; update the existing log/assert
sequence to include these three explicit assertions (start returned True,
in-transaction is True, in-transaction is False after cancel).
| Function test_manualTransactionControl($t : cs:C1710.Testing) | ||
| // #tags: transaction, manual | ||
| // #transaction: false | ||
| // This test demonstrates manual transaction control | ||
|
|
||
| $t.log("Testing manual transaction control") | ||
|
|
||
| // Start a manual transaction | ||
| var $started : Boolean | ||
| $started:=$t.startTransaction() | ||
| $t.assert.isTrue($t; $started; "Transaction started successfully") | ||
| $t.assert.isTrue($t; $t.inTransaction(); "We are in a transaction") | ||
|
|
||
| // Simulate some database operations | ||
| $t.log("Performing database operations") | ||
|
|
||
| // Cancel the transaction manually | ||
| $t.cancelTransaction() | ||
| $t.assert.isFalse($t; $t.inTransaction(); "Transaction was cancelled") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard cancellation to avoid leaking or erroneously cancelling when start fails.
If the transaction didn't start (or was already cancelled due to an earlier failure), unconditionally calling cancel may hide issues. Guard the cancellation.
Apply this diff:
// Cancel the transaction manually
- $t.cancelTransaction()
+ If ($started & $t.inTransaction())
+ $t.cancelTransaction()
+ End if
$t.assert.isFalse($t; $t.inTransaction(); "Transaction was cancelled")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Function test_manualTransactionControl($t : cs:C1710.Testing) | |
| // #tags: transaction, manual | |
| // #transaction: false | |
| // This test demonstrates manual transaction control | |
| $t.log("Testing manual transaction control") | |
| // Start a manual transaction | |
| var $started : Boolean | |
| $started:=$t.startTransaction() | |
| $t.assert.isTrue($t; $started; "Transaction started successfully") | |
| $t.assert.isTrue($t; $t.inTransaction(); "We are in a transaction") | |
| // Simulate some database operations | |
| $t.log("Performing database operations") | |
| // Cancel the transaction manually | |
| $t.cancelTransaction() | |
| $t.assert.isFalse($t; $t.inTransaction(); "Transaction was cancelled") | |
| Function test_manualTransactionControl($t : cs:C1710.Testing) | |
| // #tags: transaction, manual | |
| // #transaction: false | |
| // This test demonstrates manual transaction control | |
| $t.log("Testing manual transaction control") | |
| // Start a manual transaction | |
| var $started : Boolean | |
| $started:=$t.startTransaction() | |
| $t.assert.isTrue($t; $started; "Transaction started successfully") | |
| $t.assert.isTrue($t; $t.inTransaction(); "We are in a transaction") | |
| // Simulate some database operations | |
| $t.log("Performing database operations") | |
| // Cancel the transaction manually | |
| If ($started & $t.inTransaction()) | |
| $t.cancelTransaction() | |
| End if | |
| $t.assert.isFalse($t; $t.inTransaction(); "Transaction was cancelled") |
🤖 Prompt for AI Agents
In testing/Project/Sources/Classes/TransactionExampleTest.4dm around lines 27 to
46, the test unconditionally calls $t.cancelTransaction() even if
$t.startTransaction() failed; change the flow to guard cancellation by checking
the $started boolean (or $t.inTransaction()) before calling cancelTransaction
and making the related assertion—only call $t.cancelTransaction() and assert
that we're not in a transaction if $started is true (otherwise assert start
failed and skip cancellation) so cancellation can't run when start didn't
succeed.
| Function test_transactionWithValidation($t : cs:C1710.Testing) | ||
| // #tags: transaction, validation | ||
| // #transaction: false | ||
| // This test demonstrates transaction validation for persistent changes | ||
|
|
||
| $t.log("Testing transaction with validation") | ||
|
|
||
| // Simplified test - just verify the method exists and can be called | ||
| var $success : Boolean | ||
| $success:=$t.withTransactionValidate(Formula:C1597($t.log("Testing operation within transaction"))) | ||
|
|
||
| $t.assert.isTrue($t; $success; "Transaction validated successfully") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure the validation block returns a Boolean.
If withTransactionValidate expects the block to yield a Boolean to indicate validation success, the current block only logs. Make the block return True explicitly so $success is meaningful.
Apply this diff:
- $success:=$t.withTransactionValidate(Formula:C1597($t.log("Testing operation within transaction")))
+ $success:=$t.withTransactionValidate(Formula:C1597($t.log("Testing operation within transaction"); True))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Function test_transactionWithValidation($t : cs:C1710.Testing) | |
| // #tags: transaction, validation | |
| // #transaction: false | |
| // This test demonstrates transaction validation for persistent changes | |
| $t.log("Testing transaction with validation") | |
| // Simplified test - just verify the method exists and can be called | |
| var $success : Boolean | |
| $success:=$t.withTransactionValidate(Formula:C1597($t.log("Testing operation within transaction"))) | |
| $t.assert.isTrue($t; $success; "Transaction validated successfully") | |
| Function test_transactionWithValidation($t : cs:C1710.Testing) | |
| // #tags: transaction, validation | |
| // #transaction: false | |
| // This test demonstrates transaction validation for persistent changes | |
| $t.log("Testing transaction with validation") | |
| // Simplified test - just verify the method exists and can be called | |
| var $success : Boolean | |
| $success:=$t.withTransactionValidate(Formula:C1597($t.log("Testing operation within transaction"); True)) | |
| $t.assert.isTrue($t; $success; "Transaction validated successfully") |
🤖 Prompt for AI Agents
In testing/Project/Sources/Classes/TransactionExampleTest.4dm around lines 47 to
59, the validation lambda passed to withTransactionValidate only logs and does
not return a Boolean; modify the validation block so it yields an explicit
Boolean (e.g., perform the log, then return True) so $success receives a
meaningful Boolean result and the subsequent assert validates correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
testing/Project/Sources/Classes/Testing.4dm (5)
37-41: startTransaction should avoid nested transactions and return real status.Starting when already in a transaction risks errors/nesting anomalies. Return whether a transaction is active after the call.
Apply this diff:
Function startTransaction() : Boolean - // Start a transaction and return success status - START TRANSACTION:C239 - return True + // Start a transaction and return success status + If (In transaction:C397) + // Already in a transaction; do not start a nested one + return False + End if + START TRANSACTION:C239 + return (In transaction:C397)
42-46: validateTransaction should no-op safely when no transaction is active.Guard against calling VALIDATE when none is active; return a meaningful boolean.
Apply this diff:
Function validateTransaction() : Boolean // Validate the current transaction - VALIDATE TRANSACTION:C240 - return (OK:C209=1) + If (Not:C34(In transaction:C397)) + return False + End if + VALIDATE TRANSACTION:C240 + return (OK:C209=1)
47-50: Guard cancelTransaction to avoid spurious errors.CANCEL TRANSACTION when none is active can raise errors; add a simple check.
Apply this diff:
Function cancelTransaction() - // Cancel the current transaction - CANCEL TRANSACTION:C241 + // Cancel the current transaction (if any) + If (In transaction:C397) + CANCEL TRANSACTION:C241 + End if
55-86: withTransaction: avoid nested interference and account for runtime errors; always rollback.Current code always starts/cancels a transaction, which will cancel an outer caller’s transaction if invoked inside one. It also treats assertion-only status as success, ignoring runtime errors captured by your error handler (e.g., Storage.testErrors). Fix both and correct the comment.
Apply this diff:
Function withTransaction($operation : 4D:C1709.Function) : Boolean - // Execute an operation within a transaction - // Returns true if operation succeeded and transaction was validated + // Execute an operation within a transaction (always rolled back) + // Returns true if operation ran without assertions or runtime errors var $success : Boolean $success:=False + var $errorsBefore : Integer + var $errorsAfter : Integer + $errorsBefore:=0 + If (Storage:C1525.testErrors#Null:C1517) + $errorsBefore:=Storage:C1525.testErrors.length + End if - START TRANSACTION:C239 + // If already in a transaction, do not alter the current scope + If (In transaction:C397) + var $previousErrorHandler : Text + $previousErrorHandler:=Method called on error:C704 + ON ERR CALL:C155("TestErrorHandler") + $operation.apply() + // Restore previous error handler + If ($previousErrorHandler#"") + ON ERR CALL:C155($previousErrorHandler) + Else + ON ERR CALL:C155("") + End if + $errorsAfter:=0 + If (Storage:C1525.testErrors#Null:C1517) + $errorsAfter:=Storage:C1525.testErrors.length + End if + return (Not:C34(This:C1470.failed)) && ($errorsAfter=$errorsBefore) + End if + + START TRANSACTION:C239 // Set up error handler to catch any errors during operation var $previousErrorHandler : Text $previousErrorHandler:=Method called on error:C704 ON ERR CALL:C155("TestErrorHandler") $operation.apply() // Restore previous error handler If ($previousErrorHandler#"") ON ERR CALL:C155($previousErrorHandler) Else ON ERR CALL:C155("") End if - // Check if operation succeeded (no test failures) - If (Not:C34(This:C1470.failed)) - CANCEL TRANSACTION:C241 // Always rollback for withTransaction - $success:=True - Else - CANCEL TRANSACTION:C241 - $success:=False - End if + // Always rollback for this wrapper + If (In transaction:C397) + CANCEL TRANSACTION:C241 + End if + $errorsAfter:=0 + If (Storage:C1525.testErrors#Null:C1517) + $errorsAfter:=Storage:C1525.testErrors.length + End if + return (Not:C34(This:C1470.failed)) && ($errorsAfter=$errorsBefore) - return $success + return $success
88-119: withTransactionValidate: commit only on true success; rollback on failure/validate failure; avoid nested interference.
- Don’t start/cancel if already inside a transaction.
- Treat runtime errors (captured by the temporary error handler) as failures, not just assertions.
- If VALIDATE fails (OK≠1), ensure we CANCEL to close the transaction.
Apply this diff:
Function withTransactionValidate($operation : 4D:C1709.Function) : Boolean - // Execute an operation within a transaction and always validate on success + // Execute an operation within a transaction and validate on success // Useful for tests that need to persist data var $success : Boolean $success:=False + var $errorsBefore : Integer + var $errorsAfter : Integer + $errorsBefore:=0 + If (Storage:C1525.testErrors#Null:C1517) + $errorsBefore:=Storage:C1525.testErrors.length + End if - START TRANSACTION:C239 + // If already in a transaction, do not alter the current scope or commit state + If (In transaction:C397) + var $previousErrorHandler : Text + $previousErrorHandler:=Method called on error:C704 + ON ERR CALL:C155("TestErrorHandler") + $operation.apply() + // Restore previous error handler + If ($previousErrorHandler#"") + ON ERR CALL:C155($previousErrorHandler) + Else + ON ERR CALL:C155("") + End if + $errorsAfter:=0 + If (Storage:C1525.testErrors#Null:C1517) + $errorsAfter:=Storage:C1525.testErrors.length + End if + return (Not:C34(This:C1470.failed)) && ($errorsAfter=$errorsBefore) + End if + + START TRANSACTION:C239 // Set up error handler to catch any errors during operation var $previousErrorHandler : Text $previousErrorHandler:=Method called on error:C704 ON ERR CALL:C155("TestErrorHandler") $operation.apply() // Restore previous error handler If ($previousErrorHandler#"") ON ERR CALL:C155($previousErrorHandler) Else ON ERR CALL:C155("") End if - // Validate transaction if test succeeded - If (Not:C34(This:C1470.failed)) - VALIDATE TRANSACTION:C240 - $success:=(OK:C209=1) - Else - CANCEL TRANSACTION:C241 - $success:=False - End if + // Commit if no failures/errors; otherwise rollback + $errorsAfter:=0 + If (Storage:C1525.testErrors#Null:C1517) + $errorsAfter:=Storage:C1525.testErrors.length + End if + If (Not:C34(This:C1470.failed)) && ($errorsAfter=$errorsBefore) + VALIDATE TRANSACTION:C240 + $success:=(OK:C209=1) + // If validate failed, ensure we rollback and close + If (Not:C34($success)) && (In transaction:C397) + CANCEL TRANSACTION:C241 + End if + Else + If (In transaction:C397) + CANCEL TRANSACTION:C241 + End if + $success:=False + End if return $success
🧹 Nitpick comments (1)
testing/Project/Sources/Classes/Testing.4dm (1)
32-34: Clarify/implement run() to align with per-test transaction strategy.This stub suggests future work. Given the PR’s objective (automatic per-test transactions with opt-out), either implement here or clearly document that TestFunction.run() is the sole entry point handling auto-transactions to avoid confusion/duplication.
I can draft run() to delegate to your manual wrappers or confirm it’s intentionally unused here. Prefer?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
testing/Project/Sources/Classes/Testing.4dm(1 hunks)
🔇 Additional comments (1)
testing/Project/Sources/Classes/Testing.4dm (1)
51-54: LGTM: inTransaction provides a clean state probe.Implementation matches intended semantics and centralizes the predicate for reuse.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Change withTransaction to always call CANCEL TRANSACTION on success instead of VALIDATE TRANSACTION, matching the documented behavior that states operations "will be rolled back automatically". withTransactionValidate remains unchanged for persisting data. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
b1ba335 to
2b826bf
Compare
2b826bf to
b32e8ba
Compare
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests