IGNITE-28675 Fix flaky TxDeadlockCauseTest #testCause, #testCauseSeveralNodes

[zstan]

No description provided.

[zstan]

From documentation:
https://ignite.apache.org/docs/ignite2/latest/key-value-api/transactions
However, if a deadlock is detected, the cause of the returned TransactionTimeoutException will be TransactionDeadlockException (at least for one transaction involved in the deadlock).

[ignitetcbot]

TCBot Test Analysis

• TeamCity: --> Run :: All Results
• Branch: pull/13136/head
• Base: master
• Tested commit: 94b11ec

Possible Blockers (0)

No blockers found.

New Tests (0)

No new tests found.

[ignitetcbot]

Thanks for working on this flaky test. I have two small suggestions:

1. Please reconsider making TxDeadlockDetection.deadLockTimeout static final while TxDeadlockDetectionNoHangsTest relies on @WithSystemProperty. In the suite, earlier tests can initialize TxDeadlockDetection before the class-level property is applied, so this test may still use the default timeout. A safer option is to read IGNITE_TX_DEADLOCK_DETECTION_TIMEOUT when creating the timeout object, or keep an explicit test hook. See TxDeadlockDetection, TxDeadlockDetectionNoHangsTest, and suite order in TxDeadlockDetectionTestSuite.

2. In TxDeadlockCauseTest, the new catch stores only exceptions that already have TransactionDeadlockException. If both transaction threads fail for another reason, the test will lose the original failure and report only assertNotNull. Could we keep the first non-deadlock exception as a diagnostic fallback, but still prefer the deadlock exception when it appears? See checkCauseObject.

[zstan]

this is from our own documentation

[chesnokoff]

Let's roll back adding the final qualifier here, because it changes the semantics of TxDeadlockDetectionNoHangsTest and is not the main goal of this patch.

[chesnokoff]

and roll back renaming too

[zstan]

disagree, why do we need to support non-final definition only for test cases ?
test work properly well without this reflection changes, i run it multiple times, you can check it through TC

[chesnokoff]

My position is not really about final/non-final definition itself. My point is that it is usually safer to keep PRs as minimal as possible for a specific purpose. The main goal here is to fix TxDeadlockCauseTest, and for now it is not very clear why we also need to refactor an unrelated field and affect existing test behavior.

Green TC runs are definitely a good sign, but some flaky tests fail once in weeks or even months. That's why I'd prefer to avoid introducing additional assumptions here unless this change is really required for the fix itself.

Could you provide more reasoning on why it is safe to remove before/afterTests from TxDeadlockDetectionNoHangsTest?

[zstan]

I appent additional checks into test, plz check last commit
Also, tests restore after for hardcoded - 60000, that equal to current default but can be changed in future thus there can be a situation when test restores not a default.

[sonarqubecloud]

Quality Gate failed

Failed conditions
3 New Code Smells (required ≤ 1)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE