Skip to content

fix(test): disable the GuardDuty organization admin GuardDutyMembersIntegrationTest enables - #3348

Open
okinaka wants to merge 2 commits into
floci-io:mainfrom
okinaka:fix/guardduty-members-admin-teardown
Open

fix(test): disable the GuardDuty organization admin GuardDutyMembersIntegrationTest enables#3348
okinaka wants to merge 2 commits into
floci-io:mainfrom
okinaka:fix/guardduty-members-admin-teardown

Conversation

@okinaka

@okinaka okinaka commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Summary

GuardDutyMembersIntegrationTest enables account 444444444444 as a GuardDuty
organization admin and never disables it. Those admin accounts live in a store
shared by every @QuarkusTest class in the same Surefire fork, and Macie
answers the same /admin route, so
MacieOrganizationIntegrationTest.sharedAdminRouteKeepsGuardDutyBehavior fails
whenever it runs after this class: it asks for an empty admin list on the
GuardDuty credential scope and gets 444444444444 back.

MacieOrganizationIntegrationTest.sharedAdminRouteKeepsGuardDutyBehavior:57
  JSON path adminAccounts doesn't match.
  Expected: a collection with size <0>
    Actual: <[{adminAccountId=444444444444, adminStatus=ENABLED}]>

The admin is now disabled in @AfterEach, guarded by a flag so the teardown
runs only for the test that enabled one and a single-method run of the class
still works. The teardown goes through /admin/disable, the same route the test
used to create the state, rather than reaching for the service.
GuardDutyControllerIntegrationTest already disables what it enables, so this
was the only class leaving one behind.

Type of change

  • Bug fix (fix:)
  • New feature (feat:)
  • Breaking change (feat!: or fix!:)
  • Docs / chore

AWS Compatibility

No product code changes: this is test state that leaks between classes, in the
same shape as the DetectiveIntegrationTest teardown, and the fix follows that
one's flag-guarded @AfterEach pattern.

Reproduced and verified locally. GuardDutyMembersIntegrationTest plus
MacieOrganizationIntegrationTest in one fork fails before this change on the
assertion above and passes after (7 tests). The org-admin neighbourhood is green
as well: GuardDuty*, Macie*, Detective*, Inspector2* and SecurityHub*
run to 74 tests, no failures.

A full local ./mvnw test on this branch runs 19120 tests with one error left,
and it is not this one: ContainerPlatformDockerIntegrationTest fails on this
host because Docker 29's containerd image store reports an empty Os and
Architecture for a foreign-platform image, which is unrelated to this change.
Before the change, that same run had this Macie failure alongside it.

Checklist

  • ./mvnw test passes locally
  • New or updated integration test added
  • Commit messages follow Conventional Commits

Copilot AI lite review requested due to automatic review settings September 10, 2026 05:20
@greptile-apps

greptile-apps Bot commented Sep 10, 2026

Copy link
Copy Markdown

Greptile Summary

This PR prevents GuardDutyMembersIntegrationTest from leaking its organization administrator into other Quarkus integration tests.

  • Adds flag-guarded teardown through the same /admin/disable route used by the integration flow.
  • Clears the flag after successful cleanup so the test instance accurately reflects the released state.

Confidence Score: 5/5

The PR appears safe to merge because the test-created shared state is cleaned up without changing production behavior.

The teardown runs only after the relevant test enables the organization administrator, verifies that disabling succeeds, and then resets its guard; no actionable new issue or outstanding previous finding remains.

Important Files Changed

Filename Overview
src/test/java/io/github/hectorvent/floci/services/guardduty/GuardDutyMembersIntegrationTest.java Adds scoped cleanup for the GuardDuty organization administrator and safely resets the teardown guard after successful removal.

Reviews (3): Last reviewed commit: "test(guardduty): reset the teardown flag..." | Re-trigger Greptile

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The change is a focused test-only cleanup that addresses an identified state leak without altering product behavior.

Pull request overview

This PR fixes cross-test state leakage in the GuardDuty integration tests by ensuring an organization admin account enabled during GuardDutyMembersIntegrationTest is disabled during teardown, preventing MacieOrganizationIntegrationTest.sharedAdminRouteKeepsGuardDutyBehavior from observing leftover GuardDuty admin state via the shared /admin route.

Changes:

  • Add an @AfterEach teardown in GuardDutyMembersIntegrationTest to disable the GuardDuty organization admin account created during the test.
  • Guard the teardown with an instance flag so only the test that enabled the admin performs the disable.
File summaries
File Description
src/test/java/io/github/hectorvent/floci/services/guardduty/GuardDutyMembersIntegrationTest.java Adds flag-guarded @AfterEach cleanup to disable the org admin account enabled by the test, preventing leakage into Macie tests.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@hectorvent

Copy link
Copy Markdown
Collaborator

@okinaka main is broken at the moment. PR #3345 will fix it, you can rebase main as soon this PR get merged.

…ntegrationTest enables

GuardDutyMembersIntegrationTest enables account 444444444444 as a GuardDuty
organization admin and never disables it. Those admin accounts are shared by
every @QuarkusTest class in the same Surefire fork, and Macie answers the same
/admin route, so MacieOrganizationIntegrationTest.sharedAdminRouteKeepsGuardDutyBehavior
fails whenever it runs after this class: it asks for an empty admin list on the
GuardDuty credential scope and gets 444444444444 back.

Disable the admin in @AfterEach, guarded by a flag so the teardown only runs for
the test that enabled one and a single-method run of the class still works.
@okinaka
okinaka force-pushed the fix/guardduty-members-admin-teardown branch from dfc23f1 to 990cd1e Compare September 10, 2026 05:39
The flag survives the disable, so a switch to PER_CLASS instance lifecycle would
have the next test's teardown issue a second /admin/disable and fail on the
expected 400.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working guardduty Amazon GuardDuty

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants