-
Notifications
You must be signed in to change notification settings - Fork 309
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
Improve DescribeMachine unit test by checking that something is returned #449
base: main
Are you sure you want to change the base?
Conversation
@cgwalters would you be so kind to merge this? :) |
machine1/dbus.go
Outdated
@@ -163,7 +163,7 @@ func (c *Conn) DescribeMachine(name string) (machineProps map[string]interface{} | |||
for key, val := range dbusProps { | |||
machineProps[key] = val.Value() | |||
} | |||
return | |||
return machineProps, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I always thought this to be more of a stylistic difference rather then functional? did this really fix the bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well... I think so? maybe the original intention was to pass machineProps
by reference (maps are always passed by reference in go IIRC), I will check on Monday removing this and see if the unit test below still pass :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright, nvm you are right.
# pallotron @ devvm3927 in ~/src/go-systemd/machine1 on git:main x [9:22:08]
$ git diff
diff --git a/machine1/dbus.go b/machine1/dbus.go
index 49207ea..79c30ea 100644
--- a/machine1/dbus.go
+++ b/machine1/dbus.go
@@ -163,7 +163,7 @@ func (c *Conn) DescribeMachine(name string) (machineProps map[string]interface{}
for key, val := range dbusProps {
machineProps[key] = val.Value()
}
- return machineProps, nil
+ return
}
// KillMachine sends a signal to a machine
# pallotron @ devvm3927 in ~/src/go-systemd/machine1 on git:main x [9:22:24]
$ sudo go test -v
=== RUN TestMachine
dbus_test.go:132: machine machined-test-register-tshpleci properties: map[Class:container Id:[] Leader:4058823 Name:machined-test-register-tshpleci NetworkInterfaces:[] RootDirectory: Service:go-systemd State:running Timestamp:1729268546548624 TimestampMonotonic:269560782174 Unit:machined-test-register-tshpleci.service]
dbus_test.go:132: machine machined-test-register-with-network-segaprxj properties: map[Class:container Id:[] Leader:4058825 Name:machined-test-register-with-network-segaprxj NetworkInterfaces:[] RootDirectory: Service:go-systemd State:running Timestamp:1729268546549606 TimestampMonotonic:269560783156 Unit:machined-test-register-with-network-segaprxj.service]
dbus_test.go:132: machine machined-test-create-gvuxnxby properties: map[Class:container Id:[] Leader:4058828 Name:machined-test-create-gvuxnxby NetworkInterfaces:[] RootDirectory: Service:go-systemd State:running Timestamp:1729268546551546 TimestampMonotonic:269560785096 Unit:machine-machined\x2dtest\x2dcreate\x2dgvuxnxby.scope]
dbus_test.go:132: machine machined-test-create-with-network-uhlgksmc properties: map[Class:container Id:[] Leader:4058831 Name:machined-test-create-with-network-uhlgksmc NetworkInterfaces:[] RootDirectory: Service:go-systemd State:running Timestamp:1729268546556992 TimestampMonotonic:269560790542 Unit:machine-machined\x2dtest\x2dcreate\x2dwith\x2dnetwork\x2duhlgksmc.scope]
--- PASS: TestMachine (0.10s)
=== RUN TestImages
--- PASS: TestImages (0.00s)
PASS
ok github.com/coreos/go-systemd/v22/machine1 0.106s
I will amend this, because I still think it's handy to have the check below. does it work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for contributing, if you want to add the logging and additional checks to the test they make sense to me. Before I can approve this though we need to clean up the commit details a bit.
Squash the commits we have into one then lets name it something more appropriate.
Wdyt:
debus_test: add additional checks on machine properties
This commit fixes a bug where
machineProps
was not returned.I added some logic to the unit test: