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): Cleanup tests code reducing duplicates and add some docs #567

Merged
merged 9 commits into from
Oct 3, 2024

Conversation

3v1n0
Copy link
Collaborator

@3v1n0 3v1n0 commented Oct 2, 2024

We had almost equal tests definitions for integration tests using VHS, and since we're just about to add some more, it's time to try to factorize the code so that we don't end up having 3 times the same code being repeated, with all the maintenance issues we know.

So:

  • Reduce the VHS initialization code, by using common wrappers
  • Cleanup and drop the misunderstanding between "PAM Client" vs "PAM Exec Client" and "CLI"...
  • Move helpers code to helpers files
  • Add an Hacking document with some technical details about how our complex PAM infrastructure works and how to compile and test it.

@3v1n0 3v1n0 requested a review from a team as a code owner October 2, 2024 23:38
@codecov-commenter
Copy link

codecov-commenter commented Oct 2, 2024

Codecov Report

Attention: Patch coverage is 90.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.14%. Comparing base (e304624) to head (06c541d).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
pam/tools/pam-runner/pam-runner.go 90.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #567      +/-   ##
==========================================
- Coverage   84.53%   84.14%   -0.39%     
==========================================
  Files          79       80       +1     
  Lines        7036     7072      +36     
  Branches       75       75              
==========================================
+ Hits         5948     5951       +3     
- Misses        759      792      +33     
  Partials      329      329              

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

Copy link
Member

@denisonbarbosa denisonbarbosa left a comment

Choose a reason for hiding this comment

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

Looks good, just a small question...

pam/Hacking.md Outdated Show resolved Hide resolved
pam/integration-tests/vhs-helpers_test.go Outdated Show resolved Hide resolved
Copy link
Member

@denisonbarbosa denisonbarbosa left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. Nice work!

@3v1n0 3v1n0 force-pushed the pam-cleanup-integration-dups branch 2 times, most recently from 53413fb to d46eb62 Compare October 3, 2024 16:32
We've lots of similar code that is hard to maintain in the VHS tests, so
factorize it and use the same helpers in all the tests to manage the VHS
setup and launch.
These are not related to the cli tests anymore
It's not something that we need to build by default, being used only by
tests or for manual testing, so move it out from pam folder since it
doesn't need to access to any pam-private stuff either
We've two types of clients:
 - The PAM Application Test Client that is used to start a PAM transaction
   And now we refer to it as the PAM Runner.
 - The PAM Exec Child Client that is the app implementing the PAM module
   API through DBus.

So hopefully clarify this
…bility

Adding new options or settings to a test makes it hard to read, so
split the test cases to be in multi-line mode
@3v1n0 3v1n0 force-pushed the pam-cleanup-integration-dups branch from d46eb62 to efa8336 Compare October 3, 2024 22:08
@3v1n0 3v1n0 merged commit 86d9d82 into ubuntu:main Oct 3, 2024
8 checks passed
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.

3 participants