Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

maint(pam/integration-tests): Run all the tests with a shared authd when possible #593

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

3v1n0
Copy link
Collaborator

@3v1n0 3v1n0 commented Oct 17, 2024

Run by default all the tests using an unique instance of authd daemon running so that we can test better that all the codepaths work well with lots of concurrency.

Leaving this as a draft for now because it has dependencies on some commits of #583

UDENG-5377

@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2024

Codecov Report

Attention: Patch coverage is 85.18519% with 4 lines in your changes missing coverage. Please review.

Project coverage is 83.23%. Comparing base (e9cc1e9) to head (1493b08).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
internal/brokers/manager.go 33.33% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #593      +/-   ##
==========================================
- Coverage   83.28%   83.23%   -0.06%     
==========================================
  Files          80       80              
  Lines        8617     8630      +13     
  Branches       75       75              
==========================================
+ Hits         7177     7183       +6     
- Misses       1112     1118       +6     
- Partials      328      329       +1     

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


🚨 Try these New Features:

@3v1n0 3v1n0 force-pushed the pam-tests-shared-authd branch 2 times, most recently from 0f3f9c6 to 4bd8ec9 Compare November 21, 2024 23:24
We can then expose these cleanly
Use const variables to build expected example broker user names so that
we can be more reliable when testing them
…er names

This is valid for the standard case, while on specific cases we still
rely on manual names
… tests

So that we won't clash with other test in the suite
…l tests

So we can run a shared authd for everything
We removed the temporary dir on test cleanup but we may want to keep the
lifecycle of the daemon bound to the daemon itself instead
The user name now has only to contain the pre-check value prefix
while we don't care about its location to allow more combination of
cases (such as pre-check MFA users).
… daemon

The daemon is ref-counted in order to decide when killing it, so that
multiple tests can share the same daemon if they want to
…nces

It can be useful for debugging purposes
In case of issues it allows easier debugging
@3v1n0 3v1n0 marked this pull request as ready for review November 22, 2024 15:48
@3v1n0 3v1n0 requested a review from a team as a code owner November 22, 2024 15:48
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