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

Add steps/network tests #157

Merged
merged 2 commits into from
Oct 22, 2021

Conversation

yitsushi
Copy link
Contributor

@yitsushi yitsushi commented Oct 21, 2021

What this PR does / why we need it:

Testing network interface steps, because it had zero tests and we are before a not so small refactoring, more tests we have, more likely we can catch an error.

While I was working on #144, I found a strange code part and wrote it down as note to check back because it's not a bug, and wanted to verify it shouldn't be executed in any cases, at least my intuition told me that. The easiest way to check, writing test and yes, in out current plan model, it's a dead code.

Which issue(s) this PR fixes:

Related to #155

Special notes for your reviewer:

Added testing for a case that can never happen based on our code, but the intention seems like we want to call it, it just not on the right place or something.

In core/steps/network/interface_create.go, in the Do function, we have this block:

exists, err := s.svc.IfaceExists(ctx, deviceName)
if err != nil {
	return nil, fmt.Errorf("checking if networking interface exists: %w", err)
}
if exists {
	details, err := s.svc.IfaceDetails(ctx, deviceName)
	if err != nil {
		return nil, fmt.Errorf("getting interface details: %w", err)
	}

	s.status.HostDeviceName = deviceName
	s.status.Index = details.Index
	s.status.MACAddress = details.MAC

	return nil, nil
}

The ShouldDo returns false if the device is there, so that if exists { ... } in the Do function will never be executed.

Checklist:

  • squashed commits into logical changes
  • adds unit tests

@yitsushi yitsushi added the kind/cleanup Removing things previously overlooked label Oct 21, 2021
Copy link
Member

@Callisto13 Callisto13 left a comment

Choose a reason for hiding this comment

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

really clear and simple 🎉
just a couple of notes

core/steps/network/interface_create_test.go Show resolved Hide resolved
core/steps/network/interface_delete_test.go Show resolved Hide resolved
core/steps/network/interface_delete_test.go Outdated Show resolved Hide resolved
@yitsushi yitsushi force-pushed the 155-add-steps-network-tests branch from 973bb9d to 15f75c0 Compare October 22, 2021 13:22
Callisto13
Callisto13 previously approved these changes Oct 22, 2021
Copy link
Member

@Callisto13 Callisto13 left a comment

Choose a reason for hiding this comment

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

lgtm

before you merge, can you make all the common string values variables? for easy changing in future

@yitsushi
Copy link
Contributor Author

yitsushi commented Oct 22, 2021

@Callisto13: Moved all inline strings into constants and added a helper
function to create a new "full" interface. That was repeated multiple times,
now they are just a fullNetworkInterface() call.

Edit: Aaaand you approved before I could mention you and ask for a new
stamp. 😆

@yitsushi yitsushi merged commit 87bc452 into liquidmetal-dev:main Oct 22, 2021
@Callisto13
Copy link
Member

I happened to be lurking nearby 👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Removing things previously overlooked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants