Skip to content

Conversation

@cnathe
Copy link
Contributor

@cnathe cnathe commented Jul 31, 2025

Rationale

The SMProMoveAliquotTest.testMoveAliquotToParentProject test intermittent failure indicates that it is getting 3 audit events for the last transactionId when it only expects 2. I have a hunch that it is looking at the wrong transaction events. This PR adds some logging in this failure case.

Changes

  • AuditLogHelper.checkAuditEventDiffCountForLastTransaction debugging for unexpected number of events case

@cnathe cnathe self-assigned this Jul 31, 2025
Copy link
Member

@labkey-tchad labkey-tchad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your hunch might be correct. Even if it isn't in this case, the fact that it's possible that it might be indicates that this probably isn't a reliable way to perform this check.

It might be prudent to change how the check works at a more fundamental level. This method should take a runnable that triggers the event that is expected to be audited. That way it can know the previous state and wait for a change after triggering the event instead of assuming that the event has already completed. Something like:

checkAuditEventDiffCountForLastTransaction(/*existing params*/, Runnable runnable)
{
    Integer initialTransactionId = getLastTransactionId(containerPath, auditEventName);
    runnable.run();
    // Wait for transaction with id > initialTransactionId and verify counts
}

@cnathe
Copy link
Contributor Author

cnathe commented Aug 4, 2025

@labkey-tchad I like your idea of adding a Runnable and checking / waiting for the new transactionId to show up in the audit event before checking. I'm going to get this small change merged and then we can look into using the Runnable in a separate PR.

@cnathe cnathe merged commit f1a209e into develop Aug 4, 2025
8 of 9 checks passed
@cnathe cnathe deleted the fb_auditLogHelperDebug branch August 4, 2025 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants