Skip to content

Conversation

@tuminoid
Copy link
Member

@tuminoid tuminoid commented Nov 28, 2025

Add unit tests to SecretManager, similar to IRSO's tests in metal3-io/ironic-standalone-operator#441 . Take some, give some!

Checklist:

  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • E2E tests have been added, if necessary.
  • Integration tests have been added, if necessary.

@metal3-io-bot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@metal3-io-bot metal3-io-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 28, 2025
@metal3-io-bot metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 28, 2025
@tuminoid tuminoid requested a review from Copilot November 28, 2025 07:43
@tuminoid tuminoid marked this pull request as ready for review November 28, 2025 07:49
@metal3-io-bot metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 28, 2025
Copy link

Copilot AI left a comment

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.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@tuminoid
Copy link
Member Author

tuminoid commented Dec 4, 2025

/cc @dtantsur @elfosardo

Add unit tests to SecretManager.

Signed-off-by: Tuomo Tanskanen <[email protected]>
@tuminoid tuminoid force-pushed the tuomo/add-tests-to-secret-manager branch from 584f620 to 9fe7f7a Compare December 8, 2025 10:29
@tuminoid
Copy link
Member Author

tuminoid commented Dec 8, 2025

Fixed similar issues as in source PR, and rebased.

@kashifest
Copy link
Member

Its been long I have used these assertions. When something breaks in the code, would this output the reason of the failure as logs from the code or just the failure since I don't see any log outputs here. I mean the test itself is not generating any log but would it atleast propagate the failure log from the code itself it is testing ?

Copy link
Member

@kashifest kashifest left a comment

Choose a reason for hiding this comment

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

/approve
/hold
I have a question in line

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 11, 2025
@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kashifest

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 11, 2025
@tuminoid
Copy link
Member Author

Its been long I have used these assertions. When something breaks in the code, would this output the reason of the failure as logs from the code or just the failure since I don't see any log outputs here. I mean the test itself is not generating any log but would it atleast propagate the failure log from the code itself it is testing ?

Unit tests should never log by design. They assert, which tell the failure reason. None of the unit tests in BMO or in our other repos print logs.

@kashifest
Copy link
Member

Unit tests should never log by design. They assert, which tell the failure reason. None of the unit tests in BMO or in our other repos print logs.

Ok so it will propagate the failure reason. I was not meaning add logs here. My question was how assert handles the failure reason.

/hold cancel

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 11, 2025
@MahnoorAsghar
Copy link
Contributor

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants