Closed hunts closed 2 years ago
Hey Hunts, what do your tests look like? I think in general it might be easier to make your code take an interface{ Upgrade() error }
and then test it by mocking out the actual upgrader.
Otherwise I'd have to take a look at the implications of allowing another upgrader after Stop().
After taking another look at the code, this change looks not safe. As a 2nd upgrader would lost information about the fds and their names if there is a parent. To mock the Upgrader might be the way going ahead :(
FYI about the background anyway:
The use case is that in rrDNS, it inherits a list of fds from systemd. We call upgrader.AddFile() to track these files. When then upgrader exits, it start doing graceful shutdown that closes the TCP listeners and UDP connections.
In our tests, we'd like to test the graceful shutdown functions, by creating some TCP listeners listening on some addresses, and then terminate them, and expecting sending TCP packets to those address would return errors. However, after adding Upgrader in flow, as which also holds a reference to the underlying socket file of the listeners, we need to call upgrader.Stop() to release the fds.
Then the problem came, a stopped upgrader is not able to do anything now, but we have multiple test cases that cover difference situations that need multiple Upgrader instances.
Fwiw, what I'm doing instead is something like (simplified):
interface FileTracker {
AddFile(name string, f *os.File) error
}
type TrackerFunc func(name string, f *os.File) error
func (fn TrackerFunc) AddFile(name string, f *os.File) error {
return fn(name, f)
}
func Method1(..., ft FileTracker) {
...
if err := ft.AddFile(name, f); err !=nil {
...
}
...
}
func TestMethod1(t *testing.T) {
trackedFiles := map[string]*os.File{}
ft := TrackerFunc(func(name string, f *os.File) error) {
trackedFiles[name] = f
return nil
}
method1(..., ft)
... assert something
for _, f := range trackedFiles {
f.Close()
}
... assert something
}
This allows new instance to be created when an old instance has stopped. This is useful especially in test cases.