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

Improve DescribeMachine unit test by checking that something is returned #449

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pallotron
Copy link

This commit fixes a bug where machineProps was not returned.
I added some logic to the unit test:

$ sudo go test -v
=== RUN   TestMachine
    dbus_test.go:132: machine machined-test-register-gkkxchal properties: map[Class:container Id:[] Leader:435704 Name:machined-test-register-gkkxchal NetworkInterfaces:[] RootDirectory: Service:go-systemd State:running Timestamp:1728652846963827 TimestampMonotonic:6097688654967 Unit:machined-test-register-gkkxchal.service]
    dbus_test.go:132: machine machined-test-register-with-network-bfeyiref properties: map[Class:container Id:[] Leader:435706 Name:machined-test-register-with-network-bfeyiref NetworkInterfaces:[] RootDirectory: Service:go-systemd State:running Timestamp:1728652846964826 TimestampMonotonic:6097688655967 Unit:machined-test-register-with-network-bfeyiref.service]
    dbus_test.go:132: machine machined-test-create-thdkckgj properties: map[Class:container Id:[] Leader:435708 Name:machined-test-create-thdkckgj NetworkInterfaces:[] RootDirectory: Service:go-systemd State:running Timestamp:1728652846966834 TimestampMonotonic:6097688657975 Unit:machine-machined\x2dtest\x2dcreate\x2dthdkckgj.scope]
    dbus_test.go:132: machine machined-test-create-with-network-mkqmytyw properties: map[Class:container Id:[] Leader:435710 Name:machined-test-create-with-network-mkqmytyw NetworkInterfaces:[] RootDirectory: Service:go-systemd State:running Timestamp:1728652846972931 TimestampMonotonic:6097688664072 Unit:machine-machined\x2dtest\x2dcreate\x2dwith\x2dnetwork\x2dmkqmytyw.scope]
--- PASS: TestMachine (0.13s)
=== RUN   TestImages
--- PASS: TestImages (0.00s)
PASS
ok      github.com/coreos/go-systemd/v22/machine1       0.134s

@pallotron
Copy link
Author

@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

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?

Copy link
Author

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 :)

Copy link
Author

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?

Copy link
Author

Choose a reason for hiding this comment

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

done

@pallotron pallotron changed the title add missing return of properties in DescribeMachine Improve DescribeMachine unit test by checking that something is returned Oct 18, 2024
Copy link

@prestist prestist left a 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

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.

2 participants