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.

Sign up for GitHub

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

Jump to bottom

Add unit tests #46

Merged
merged 2 commits into from
May 15, 2024
Merged

Add unit tests #46

merged 2 commits into from
May 15, 2024

Conversation

p-strusiewiczsurmacki-mobica
Copy link
Contributor

@p-strusiewiczsurmacki-mobica p-strusiewiczsurmacki-mobica commented Aug 30, 2023 •
edited
Loading

This is PR for unit tests.

The biggest change here is creation of additional interface in pkg/nl/interface.go that will be later used as intermediate layer between our packages and netlink library. This way we can mock netlink calls using gomock.

For pkg/frr DBus interface was introduced to fulfill the same goal.

Current state:

ok       github.com/telekom/das-schiff-network-operator/pkg/anycast      0.024s  coverage: 93.7% of statements
ok      github.com/telekom/das-schiff-network-operator/pkg/config       0.028s  coverage: 100.0% of statements
ok      github.com/telekom/das-schiff-network-operator/pkg/debounce     0.036s  coverage: 7.1% of statements
ok      github.com/telekom/das-schiff-network-operator/pkg/frr  0.027s  coverage: 25.1% of statements
ok      github.com/telekom/das-schiff-network-operator/pkg/healthcheck  0.043s  coverage: 100.0% of statements
ok      github.com/telekom/das-schiff-network-operator/pkg/nl   23.575s coverage: 68.2% of statements

@p-strusiewiczsurmacki-mobica p-strusiewiczsurmacki-mobica changed the title [WIP] Add unit tests Add unit tests Sep 5, 2023
@p-strusiewiczsurmacki-mobica p-strusiewiczsurmacki-mobica force-pushed the unit-tests branch 3 times, most recently from 2ae54e3 to fb32b5b Compare September 15, 2023 13:19
schrej
schrej requested changes Oct 4, 2023
View reviewed changes
pkg/anycast/anycast_test.go Outdated
Comment on lines 20 to 22
var _ = BeforeSuite(func() {

})
Copy link
Member

Choose a reason for hiding this comment

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

rm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

pkg/anycast/anycast_test.go Outdated
}

var _ = Describe("anycast", func() {
Context("buildRoute() should", func() {
Copy link
Member

Choose a reason for hiding this comment

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

I would remove the "should" here. If it's about flow when reading, you can consider replacing Context with another Describe, so it can be read as "Describe buildRoute()".
Describe, Context and When all do the same thing, they just exist to make it easier to read as sentences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

pkg/anycast/anycast_test.go Outdated

var _ = Describe("anycast", func() {
Context("buildRoute() should", func() {
It("build route", func() {
Copy link
Member

Choose a reason for hiding this comment

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

nit: should be "builds route", which allows reading it as "It builds route".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

pkg/anycast/anycast_test.go Outdated
"Anycast Suite")
}

var _ = Describe("anycast", func() {
Copy link
Member

Choose a reason for hiding this comment

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

not sure if we need this wrapper, we could just have var _ = Describe() for all the Context()s that are contained in this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Removed also in other tests.

pkg/config/config_test.go Outdated
Comment on lines 13 to 15
var _ = BeforeSuite(func() {

})
Copy link
Member

Choose a reason for hiding this comment

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

rm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

pkg/frr/dbus/dbus.go Outdated
Comment on lines 13 to 15
}

type ConnInterface interface {
Copy link
Member

Choose a reason for hiding this comment

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

If there is nothing else using the name Connection, I'd just call it that instead of adding the Interface suffix. Same for System.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

pkg/nl/manager.go Outdated
@@ -26,9 +26,14 @@ const (
var macPrefix = []byte("\x02\x54")

type NetlinkManager struct {
Copy link
Member

Choose a reason for hiding this comment

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

even though this isn't part of the MR, this should be called Manager since the package is already called netlink (well, nl).
https://go.dev/doc/effective_go#names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

pkg/nl/interface.go Outdated
LinkSetMaster(link netlink.Link, master netlink.Link) error
}

type NetlinkToolkit struct{}
Copy link
Member

Choose a reason for hiding this comment

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

should just be called Toolkit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

schrej
schrej approved these changes Apr 17, 2024
View reviewed changes
Copy link
Member

@schrej schrej left a comment

Choose a reason for hiding this comment

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

Sorry for ignoring this for so long. Could you rebase? Maybe after the other large ones are in. Good from my side, everything was basically just minor or nits anyway.

@p-strusiewiczsurmacki-mobica
Added unit tests
ffcf9dc 
Signed-off-by: Patryk Strusiewicz-Surmacki <[email protected]>
@p-strusiewiczsurmacki-mobica
Fixed rebase issues and removed CreateL2 tests
bdab15e 
Signed-off-by: Patryk Strusiewicz-Surmacki <[email protected]>
@schrej schrej merged commit daeaedc into telekom:main May 15, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schrej schrej schrej approved these changes

@MaxRink MaxRink Awaiting requested review from MaxRink MaxRink is a code owner

@Cellebyte Cellebyte Awaiting requested review from Cellebyte Cellebyte is a code owner

@chdxD1 chdxD1 Awaiting requested review from chdxD1 chdxD1 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@p-strusiewiczsurmacki-mobica @schrej