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

Ensure there's sufficient testing for the "lookupPaths" in the creds manager #8897

Open
Spimtav opened this issue Jan 23, 2024 · 0 comments
Open

Comments

@Spimtav
Copy link
Contributor

Spimtav commented Jan 23, 2024

Summary

Rui and i were discussing this PR he asked me to review, and specifically this test here in the ssm_test.go.

In my initial review, i raised the concern that the new SharedPaths feature wasn't being sufficiently tested because there only seemed to be a happy path test and no sad paths. In the ensuing discussion, we hit on the following points which Rui asked me to write down to jog our memories later:

  • that this new feature hooked into a shared modular unit, ie "lookupPaths",
  • that "lookupPaths" was being implemented and tested sufficiently by itself,
  • that the test in question in the PR was just to make sure it hooked up into the shared modular unit correctly,
  • my stance that we should explicitly have a unit test in ssm_test.go to make sure that it hooks into the shared modular unit, and also that we could just rely on the shared modular unit to do all the happy and sad path testing for us
  • that Rui pointed out all the other credential managers have this exact single happy path integration test, and that the PR author probably just copied it from there
  • Rui's worry that perhaps there actually wasn't sufficient test coverage on the shared modular unit at all.

As such, Rui asked me to write this issue so we could investigate later whether or not the shared modular unit (lookupPaths) actually does have sufficient testing. Rui did a quick search with me and couldn't actually find tests for it, so we should revisit this later and make sure it's being tested properly.

Also tagging @xtremerui just so this pops up in your feed, even though you're watching me type this lol.

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

No branches or pull requests

1 participant