Skip to content

Log orphaned attachments action for tests#2940

Open
labkey-adam wants to merge 1 commit intodevelopfrom
fb_log_orphans
Open

Log orphaned attachments action for tests#2940
labkey-adam wants to merge 1 commit intodevelopfrom
fb_log_orphans

Conversation

@labkey-adam
Copy link
Copy Markdown
Contributor

Rationale

AttachmentHelper.logOrphanedAttachments() gives the tests a way to proactively check for orphaned attachments

Related Pull Requests

Copy link
Copy Markdown
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'd be happy to have this check run after every test.

Comment on lines +66 to +73
@Test
public void testOrphanedAttachments()
{
int orphanCount = AttachmentHelper.logOrphanedAttachments();
if (orphanCount > 0)
log("Orphaned attachments: " + orphanCount);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The test runner always forces BasicTest to run first so this won't catch anything on TeamCity. I'd suggest adding this check to BaseWebDriverTest.doPostamble, around where we run the crawler (checkLinks) and check for CSP warnings (checkNewCspWarnings).
It should also throw an error if any orphaned attachments are found, otherwise we won't ever notice them.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually, I didn't notice that the logOrphanedAttachments action will make ERROR logs. Those will already cause the test to fail so there's no need to add the extra fail case from the orphaned attachment check itself.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can remove the check from BasicTest. The product still checks and logs errors for orphaned attachments before every container delete, so will doPostamble check anything that delete container won't?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh. Yeah, adding the check to doPostamble wouldn't accomplish much in that case. I thought the existing orphaned attachment check happened after container deletion and that we were adding an extra check for items that are cleaned up by container deletion but might be orphaned by other means.

Copy link
Copy Markdown
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.

Maybe add testOrphanedAttachments to DatabaseDiagnosticsTest instead. It's in a lot of suites and is always the final test of a suite.

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.

2 participants