test: improve systemd test coverage #254
Conversation
jlocash
commented
Aug 7, 2025
jlocash
commented
- Card ID: CCT-1100
- Adds tests covering new connections
- Adds tests covering basic unit operations operations on valid, invalid, and non-existent units:
jlocash
force-pushed
the
jlocash/add-systemd-unit-tests
branch
from
47df34a to
a11f23c
Compare
There was a problem hiding this comment.
Thanks for the PR. Overall it is OK. I have few comments and suggestions. In general please call conn.Close() in closure like this:
defer func() {
err = conn.Close()
if err != nil {
// Handle the error somehow.
// Probably something like this `t.Errorf(err)`
}
}()You use different patterns for handling error. I think that we should use one pattern for all checks. Something like this:
t.Run("start_invalid_unit", func(t *testing.T) {
err := conn.StartUnit(unitName, false)
if err == nil {
t.Error("expected error when starting invalid unit")
} else {
t.Logf("expected error returned: %v", err)
}
})If it is OK that error is returned as well error is not returned from some function call, then it should be explained in the comment.
internal/systemd/systemd_test.go
Outdated
| }) | ||
|
|
||
| t.Run("disable_nonexistent", func(t *testing.T) { | ||
| if err = conn.DisableUnit(unitName, false, true); err == nil { |
There was a problem hiding this comment.
Please, use the same pattern for all checks.
internal/systemd/systemd_test.go
Outdated
| if err == nil { | ||
| t.Logf("GetUnitState for non-existent unit returned: %q", state) | ||
| } else { | ||
| t.Logf("GetUnitState failed for non-existent unit: %v", err) | ||
| } |
There was a problem hiding this comment.
Shouldn't we call t.Error() in the case, when err == nil.
There was a problem hiding this comment.
systemd will return "inactive" for non-existent units:
# systemctl show does-not-exist.service --property=ActiveState
ActiveState=inactive
This test should have an error condition though, so I'll add one
internal/systemd/systemd_test.go
Outdated
| if err != nil { | ||
| t.Logf("GetUnitState failed for invalid unit: %v", err) | ||
| } else { | ||
| t.Logf("GetUnitState returned: %q", state) | ||
| } |
There was a problem hiding this comment.
Why the t.Error() is not called in one of the case?
There was a problem hiding this comment.
Systemd will not return an error when enabling or retrieving the ActiveState of an invalid unit (bad syntax in the unit file). It will however return an error when trying to start it. I'll update these tests to better reflect that behavior
| } | ||
| } | ||
|
|
||
| func TestUnitOperationsValid(t *testing.T) { |
There was a problem hiding this comment.
Please add here some comment saying that all functions calls should not return error in normal situation.
| }) | ||
| } | ||
|
|
||
| func TestUnitOperationsInvalid(t *testing.T) { |
There was a problem hiding this comment.
Please write some comment here explaining, why we test it. What is expected. Why some function should return error and why some should not return error.
jlocash
force-pushed
the
jlocash/add-systemd-unit-tests
branch
from
a11f23c to
330ac94
Compare
internal/systemd/systemd_test.go
Outdated
| unitName := filepath.Base(unitFile) | ||
|
|
||
| t.Run("enable_unit", func(t *testing.T) { | ||
| if err = conn.EnableUnit(unitName, false, true); err != nil { |
There was a problem hiding this comment.
Please use only one patter. E.g. this:
err = conn.EnableUnit(unitName, false, true)
if err != nil {
...
}You mix two patterns and it is harder to read.
| ) | ||
|
|
||
| func TestUnit(t *testing.T) { | ||
| func TestNewConnectionContext(t *testing.T) { |
There was a problem hiding this comment.
Please write comment here explaining purpose of this unit test.
internal/systemd/systemd_test.go
Outdated
| if err != nil { | ||
| t.Fatalf("unexpected error when getting unit state: %v", err) | ||
| } | ||
| if state == "active" { |
There was a problem hiding this comment.
I would rather change this condition to state != inactive to follow pattern from previous tests.
internal/systemd/systemd_test.go
Outdated
| } | ||
| }) | ||
|
|
||
| // systemd will return an ActiveState of "inactive" for invalid/non-existent units |
There was a problem hiding this comment.
You duplicated this comment three times in this file and it does not make too much sense. What is ActiveState? I would rephrase it to something like this: // systemd will return an "inactive" state for invalid/non-existent units
There was a problem hiding this comment.
ActiveState is the systemd unit property name, but I will rephrase it like you suggested
* Card ID: CCT-1100 * Adds tests covering new connections * Adds tests covering basic unit operations operations on valid, invalid, and non-existent units Signed-off-by: Joshua Locash <[email protected]>
jlocash
force-pushed
the
jlocash/add-systemd-unit-tests
branch
from
330ac94 to
97b7641
Compare
