Skip to content

o/i/apparmorprompting: fix potential races when logging in tests - #17023

Merged
olivercalder merged 1 commit into
canonical:masterfrom
olivercalder:prompting-fix-test-logging-races
May 5, 2026
Merged

o/i/apparmorprompting: fix potential races when logging in tests#17023
olivercalder merged 1 commit into
canonical:masterfrom
olivercalder:prompting-fix-test-logging-races

Conversation

@olivercalder

Copy link
Copy Markdown
Member

Small fixes to address potential race conditions around logging causing sporadic test failures.

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
@olivercalder olivercalder added Simple 😃 A small PR which can be reviewed quickly Test Robustness Skip spread Indicate that spread job should not run labels May 5, 2026

@andrewphelpsj andrewphelpsj left a comment

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.

Thanks!

c.Errorf("manager should still be ready")
}

time.Sleep(50 * time.Millisecond)

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.

lol what was this doing here

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Basically what it said on the tin, waiting to give a chance for the message to be logged (though it shouldn't be). There's some annoyingness with the synchronization with mocked listener here, we don't have a synchronization point other than the run loop in o/i/a/prompting.go, so triggering the listener readiness isn't sufficient to know that the run loop has picked up the event and handled it.

But the nice thing is, mgr.Stop() is a synchronization point, since it kills the tomb and waits for it. And <-m.tomb.Dying() is in the run loop select, so if the manager stops, we know that the prior event is done being handled.

@natibek natibek 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.

Thanks!

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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@codecov

codecov Bot commented May 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.07%. Comparing base (4620040) to head (69f5194).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #17023      +/-   ##
==========================================
+ Coverage   79.06%   79.07%   +0.01%     
==========================================
  Files        1374     1376       +2     
  Lines      191076   191130      +54     
  Branches     2465     2465              
==========================================
+ Hits       151065   151127      +62     
+ Misses      30918    30899      -19     
- Partials     9093     9104      +11     
Flag Coverage Δ
unittests 79.07% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

github-actions Bot commented May 5, 2026

Copy link
Copy Markdown

Tue May 5 17:46:36 UTC 2026

Spread tests skipped

@Rnfudge02 Rnfudge02 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.

LGTM!

@olivercalder
olivercalder merged commit 8152be4 into canonical:master May 5, 2026
118 of 120 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Simple 😃 A small PR which can be reviewed quickly Skip spread Indicate that spread job should not run Test Robustness

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants