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

chore: Add unit tests for nv-ipam-cni objects rendering and sync #995

Merged
merged 1 commit into from
Jul 28, 2024

Conversation

souleb
Copy link
Contributor

@souleb souleb commented Jul 16, 2024

No description provided.

@coveralls
Copy link
Collaborator

coveralls commented Jul 16, 2024

Pull Request Test Coverage Report for Build 10093836643

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 4 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+1.3%) to 64.801%

Files with Coverage Reduction New Missed Lines %
pkg/state/state_hostdevice_network.go 2 78.38%
controllers/hostdevicenetwork_controller.go 2 89.77%
Totals Coverage Status
Change from base Build 10091016261: 1.3%
Covered Lines: 3299
Relevant Lines: 5091

💛 - Coveralls

@souleb souleb force-pushed the state-nv-ipam-cni-unit-test branch 2 times, most recently from 5279fbb to 40c19aa Compare July 16, 2024 15:32
@souleb souleb changed the title Add unit tests for nv-ipam-cni objects rendering and sync chore: Add unit tests for nv-ipam-cni objects rendering and sync Jul 16, 2024
})

Context("Verify objects rendering", func() {
It("should create Daemonset - minimal spec", func() {
Copy link
Member

@tobiasgiese tobiasgiese Jul 17, 2024

Choose a reason for hiding this comment

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

I know we don't have this in our other tests (AFAIK), but should we start adding negative tests?

Copy link
Member

Choose a reason for hiding this comment

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

NB: this is just a reminder, nothing related to this PR :)

@souleb souleb force-pushed the state-nv-ipam-cni-unit-test branch 2 times, most recently from 100e188 to 451b4a2 Compare July 17, 2024 14:06
Copy link
Contributor

@killianmuldoon killianmuldoon 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 some comments around simplifying it a bit

@souleb souleb force-pushed the state-nv-ipam-cni-unit-test branch 2 times, most recently from f651916 to ed10820 Compare July 18, 2024 12:37
pkg/state/state_nv_ipam_cni.go Outdated Show resolved Hide resolved
pkg/state/state_nv_ipam_cni_test.go Outdated Show resolved Hide resolved
@souleb souleb force-pushed the state-nv-ipam-cni-unit-test branch 2 times, most recently from 08d5f65 to 0db488a Compare July 23, 2024 08:46
@souleb souleb requested a review from e0ne July 23, 2024 08:48
@souleb souleb force-pushed the state-nv-ipam-cni-unit-test branch 4 times, most recently from 73ed0db to bfac8b6 Compare July 25, 2024 12:08
@souleb souleb force-pushed the state-nv-ipam-cni-unit-test branch from bfac8b6 to 4dddd3c Compare July 25, 2024 12:11
Copy link
Collaborator

@e0ne e0ne left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for addressing my comments

@rollandf rollandf merged commit 34b58b9 into Mellanox:master Jul 28, 2024
16 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.

6 participants