tests(pam/testutils): Integration tests for the PAM CLI #109

denisonbarbosa · 2023-11-21T15:29:53Z

This adds integration tests for the PAM module and moves some code around to avoid copying and maintain consistency between our integration tests.
The golden files are ASCII representations of terminal screenshots, so don't get scared by the number of lines changed (I promise it will be okay).

UDENG-1456

denisonbarbosa · 2023-11-21T15:31:56Z

I could extract the testutils changes to another pull request if they seem too much for this one. I didn't do it yet because I thought it would feel like an "empty" change in main.

codecov-commenter · 2023-11-27T17:47:34Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (8b3c1cd) 88.62% compared to head (23d4351) 83.21%.
Report is 43 commits behind head on main.

Files	Patch %	Lines
internal/users/withgpasswdmock.go	77.77%	1 Missing and 1 partial ⚠️
internal/brokers/examplebroker/broker.go	97.43%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #109      +/-   ##
==========================================
- Coverage   88.62%   83.21%   -5.42%     
==========================================
  Files          34       56      +22     
  Lines        2532     4473    +1941     
==========================================
+ Hits         2244     3722    +1478     
- Misses        221      583     +362     
- Partials       67      168     +101

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

GabrielNagy

Great work! Nice to see the vhs thing working in the end. I left my comments inline

.github/workflows/qa.yaml

pam/integration-tests/integration_test.go

pam/integration-tests/testdata/tapes/mandatory_password_reset.tape

pam/integration-tests/testdata/tapes/switch_auth_mode.tape

pam/integration-tests/testdata/tapes/mfa_auth.tape

pam/integration-tests/integration_test.go

internal/testutils/path.go

3v1n0 · 2023-11-29T19:41:54Z

I didn't go through the vhs docs yet, but maybe it would be convenient adding a README.md to the testdata folder how to generate or update the recorded files?

denisonbarbosa · 2023-11-29T23:25:02Z

I didn't go through the vhs docs yet, but maybe it would be convenient adding a README.md to the testdata folder how to generate or update the recorded files?

I don't really like adding assets that are not related to the tests themselves inside testdata/. If you feel like it's necessary to point to the vhs documentation, I could add a line comment inside the TestIntegration function (before spawning the vhs subprocess) explaining that we rely on the tool and pointing to their documentation, WDYT?

3v1n0 · 2023-11-29T23:27:33Z

I didn't go through the vhs docs yet, but maybe it would be convenient adding a README.md to the testdata folder how to generate or update the recorded files?

I don't really like adding assets that are not related to the tests themselves inside testdata/. If you feel like it's necessary to point to the vhs documentation, I could add a line comment inside the TestIntegration function (before spawning the vhs subprocess) explaining that we rely on the tool and pointing to their documentation, WDYT?

Well I'm not sure where is the right place, in other projects I contribute or mantain we normally keep a readme in the places the artifacts are so that it's easy for everyone to find out how those are generated.

However, I'm fine with other options too, I just think it's important to have the process documented so that when others will have to regenerate them can figure it out easily.

didrocks · 2023-11-30T06:54:53Z

I didn't go through the vhs docs yet, but maybe it would be convenient adding a README.md to the testdata folder how to generate or update the recorded files?

I don't really like adding assets that are not related to the tests themselves inside testdata/. If you feel like it's necessary to point to the vhs documentation, I could add a line comment inside the TestIntegration function (before spawning the vhs subprocess) explaining that we rely on the tool and pointing to their documentation, WDYT?

Well I'm not sure where is the right place, in other projects I contribute or mantain we normally keep a readme in the places the artifacts are so that it's easy for everyone to find out how those are generated.

However, I'm fine with other options too, I just think it's important to have the process documented so that when others will have to regenerate them can figure it out easily.

Note: I didn’t look at any comments than this exchange.

I think this is the perfect place for the contributing guide on "how to refresh blablabla" (same for golden files btw)

internal/testutils/daemon.go

GabrielNagy

Great job on this! Can we ensure debian packaging works properly with these changes as well?

pam/integration-tests/integration_test.go

3v1n0 · 2023-12-13T19:28:30Z

I didn't go through the vhs docs yet, but maybe it would be convenient adding a README.md to the testdata folder how to generate or update the recorded files?

I don't really like adding assets that are not related to the tests themselves inside testdata/. If you feel like it's necessary to point to the vhs documentation, I could add a line comment inside the TestIntegration function (before spawning the vhs subprocess) explaining that we rely on the tool and pointing to their documentation, WDYT?

Well I'm not sure where is the right place, in other projects I contribute or mantain we normally keep a readme in the places the artifacts are so that it's easy for everyone to find out how those are generated.
However, I'm fine with other options too, I just think it's important to have the process documented so that when others will have to regenerate them can figure it out easily.

Note: I didn’t look at any comments than this exchange.

I think this is the perfect place for the contributing guide on "how to refresh blablabla" (same for golden files btw)

@denisonbarbosa I was quickly grepping on the diff and I didn't notice anything related to this, is the process documented now?

3v1n0

I think we're there, I've spotted few tiny things.

pam/integration-tests/integration_test.go

internal/testutils/daemon.go

.github/workflows/qa.yaml

didrocks

I think I was able to review every changes since the rebase. I have only 2 minor comments and then, we’ll be good from my side of things!

internal/users/withoutgpasswdmock.go

.github/workflows/qa.yaml

didrocks

All good from my side! Of course, needs to solve the conflict while rebasing, but otherwise, good! :)

GabrielNagy

Looks good to me, great work here!

debian/rules

didrocks

I think Gabriel is right, we need to skip them during autopkgtests too.

debian/rules

didrocks

Great work! Once fixing the conflicts and rebase, you can merge :)

CONTRIBUTING.md

We wrote some helper functions for the NSS integration tests, but they will be helpful for the PAM integration tests as well. So, in order to avoid copying code and then having to maintain both helpers, it's better to move the helpers to testutils.

We need to install vhs and ttyd to run the integration tests in the CI and also export an environment variable to build the PAM module without any warnings

The CLI tests rely on external dependencies to run and they are not available in the archive, so we must skip them on environments that do not support external sources.

Since we rely on the mock to test some behaviors in the CLI tests, we have to be able to build the binary with the mock. This now is doable by using the "integrationtests" build tag.

denisonbarbosa force-pushed the integration-tests-pam branch 13 times, most recently from 5385449 to 6619df5 Compare November 27, 2023 17:41

denisonbarbosa marked this pull request as ready for review November 27, 2023 17:46

denisonbarbosa requested a review from a team as a code owner November 27, 2023 17:46

GabrielNagy reviewed Nov 28, 2023

View reviewed changes

pam/integration-tests/testdata/tapes/switch_auth_mode.tape Outdated Show resolved Hide resolved

GabrielNagy reviewed Nov 28, 2023

View reviewed changes

pam/integration-tests/testdata/tapes/mfa_auth.tape Outdated Show resolved Hide resolved

denisonbarbosa force-pushed the integration-tests-pam branch from 6619df5 to 0b5edd5 Compare November 29, 2023 10:47

denisonbarbosa requested a review from GabrielNagy November 29, 2023 11:18

GabrielNagy approved these changes Nov 29, 2023

View reviewed changes

3v1n0 reviewed Nov 29, 2023

View reviewed changes

pam/integration-tests/integration_test.go Outdated Show resolved Hide resolved

3v1n0 reviewed Nov 29, 2023

View reviewed changes

internal/testutils/path.go Show resolved Hide resolved

3v1n0 reviewed Nov 30, 2023

View reviewed changes

internal/testutils/daemon.go Show resolved Hide resolved

denisonbarbosa force-pushed the integration-tests-pam branch from 35baf76 to 9116345 Compare December 12, 2023 15:14

denisonbarbosa requested review from didrocks and GabrielNagy December 13, 2023 09:56

GabrielNagy approved these changes Dec 13, 2023

View reviewed changes

pam/integration-tests/integration_test.go Outdated Show resolved Hide resolved

3v1n0 requested changes Dec 14, 2023

View reviewed changes

pam/integration-tests/integration_test.go Show resolved Hide resolved

internal/testutils/daemon.go Outdated Show resolved Hide resolved

.github/workflows/qa.yaml Outdated Show resolved Hide resolved

denisonbarbosa force-pushed the integration-tests-pam branch from 0de75ce to 23d4351 Compare January 2, 2024 14:08

didrocks requested changes Jan 8, 2024

View reviewed changes

internal/users/withoutgpasswdmock.go Outdated Show resolved Hide resolved

.github/workflows/qa.yaml Show resolved Hide resolved

denisonbarbosa requested a review from didrocks January 9, 2024 15:43

didrocks approved these changes Jan 9, 2024

View reviewed changes

GabrielNagy approved these changes Jan 10, 2024

View reviewed changes

GabrielNagy reviewed Jan 11, 2024

View reviewed changes

debian/rules Show resolved Hide resolved

didrocks requested changes Jan 11, 2024

View reviewed changes

debian/rules Show resolved Hide resolved

denisonbarbosa requested a review from didrocks January 11, 2024 12:15

GabrielNagy approved these changes Jan 11, 2024

View reviewed changes

didrocks approved these changes Jan 11, 2024

View reviewed changes

3v1n0 reviewed Jan 11, 2024

View reviewed changes

CONTRIBUTING.md Show resolved Hide resolved

denisonbarbosa added 2 commits January 11, 2024 10:34

Move helper functions to testutils

f95a463

We wrote some helper functions for the NSS integration tests, but they will be helpful for the PAM integration tests as well. So, in order to avoid copying code and then having to maintain both helpers, it's better to move the helpers to testutils.

Rename NSS integration tests for consistency

f860cfa

denisonbarbosa force-pushed the integration-tests-pam branch from cbb5e59 to 5fc6623 Compare January 11, 2024 14:36

Update examplebroker values for the tests

44a89f8

denisonbarbosa force-pushed the integration-tests-pam branch from 5fc6623 to 7575eee Compare January 11, 2024 14:41

denisonbarbosa added 5 commits January 11, 2024 11:02

Update CI dependencies and fix PAM build

c17fe30

We need to install vhs and ttyd to run the integration tests in the CI and also export an environment variable to build the PAM module without any warnings

Add integration tests for the PAM CLI

de2f812

Skip CLI tests during autopkgtests and pkg building

49a6e6d

The CLI tests rely on external dependencies to run and they are not available in the archive, so we must skip them on environments that do not support external sources.

Update docs to point to vhs documentation

06c7747

Add option to use gpasswd mock when building authd

10d92c3

Since we rely on the mock to test some behaviors in the CLI tests, we have to be able to build the binary with the mock. This now is doable by using the "integrationtests" build tag.

denisonbarbosa force-pushed the integration-tests-pam branch from 7575eee to 10d92c3 Compare January 11, 2024 15:04

denisonbarbosa merged commit edc6202 into main Jan 11, 2024
5 checks passed

denisonbarbosa deleted the integration-tests-pam branch January 11, 2024 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests(pam/testutils): Integration tests for the PAM CLI #109

tests(pam/testutils): Integration tests for the PAM CLI #109

denisonbarbosa commented Nov 21, 2023

denisonbarbosa commented Nov 21, 2023 •

edited

Loading

codecov-commenter commented Nov 27, 2023 •

edited

Loading

GabrielNagy left a comment

3v1n0 commented Nov 29, 2023

denisonbarbosa commented Nov 29, 2023

3v1n0 commented Nov 29, 2023

didrocks commented Nov 30, 2023

GabrielNagy left a comment

3v1n0 commented Dec 13, 2023

3v1n0 left a comment

didrocks left a comment

didrocks left a comment

GabrielNagy left a comment

didrocks left a comment

didrocks left a comment

tests(pam/testutils): Integration tests for the PAM CLI #109

tests(pam/testutils): Integration tests for the PAM CLI #109

Conversation

denisonbarbosa commented Nov 21, 2023

denisonbarbosa commented Nov 21, 2023 • edited Loading

codecov-commenter commented Nov 27, 2023 • edited Loading

Codecov Report

GabrielNagy left a comment

Choose a reason for hiding this comment

3v1n0 commented Nov 29, 2023

denisonbarbosa commented Nov 29, 2023

3v1n0 commented Nov 29, 2023

didrocks commented Nov 30, 2023

GabrielNagy left a comment

Choose a reason for hiding this comment

3v1n0 commented Dec 13, 2023

3v1n0 left a comment

Choose a reason for hiding this comment

didrocks left a comment

Choose a reason for hiding this comment

didrocks left a comment

Choose a reason for hiding this comment

GabrielNagy left a comment

Choose a reason for hiding this comment

didrocks left a comment

Choose a reason for hiding this comment

didrocks left a comment

Choose a reason for hiding this comment

denisonbarbosa commented Nov 21, 2023 •

edited

Loading

codecov-commenter commented Nov 27, 2023 •

edited

Loading