testtime: add mockable timers for use in tests #14672

olivercalder · 2024-10-24T21:55:07Z

Introduce a testtime package with a Timer interface which can be used in place of time.Timer when we want direct control over timers in tests.

The testtime.Timer interface is fully compatible with time.Timer, except that the former has a C() method instead of instance variable, as interfaces cannot have instance variables. As such, time.Timer does not directly implement testtime.Timer, so we must define a minimal wrapper (testtime.RealTimer) which directly calls the functions/methods associated with time.Timer, and exposes the latter's C instance variable as C().

The testtime.TestTimer struct simulates the behavior of time.Timer, except that control of the passage of time and the ability to fire the timer are provided directly to the user. This way, tests which depend on timer behavior can avoid race conditions and sleeps.

codecov · 2024-10-24T22:12:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.02%. Comparing base (96ea7b0) to head (1612bba).
Report is 71 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #14672      +/-   ##
==========================================
+ Coverage   78.95%   79.02%   +0.07%     
==========================================
  Files        1084     1087       +3     
  Lines      146638   147570     +932     
==========================================
+ Hits       115773   116624     +851     
- Misses      23667    23719      +52     
- Partials     7198     7227      +29

Flag	Coverage Δ
unittests	`79.02% <100.00%> (+0.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

olivercalder · 2024-10-24T22:45:45Z

It may be worth MockTimers returning a []*testtime.TestTimer so that callers can get access to the returned timers without needing to do extra mocking at the AfterFunc or NewTimer call sites.

For example, see the extra gymnastics performed here to extract the timer returned by AfterFunc: https://github.com/canonical/snapd/pull/14390/files#diff-17860faacfdc1f07fe1cdd14ecdcf6869209d498f378916ffc33a57039979e17R1084-R1094

Returning a []*testtime.TestTimer is probably fine, akin to what logger.MockLogger does. However, it would be much preferable to have a thread-safe buffer which can be permanently consumed on the reader end while being written on the writer end. Like a channel with growable buffer space.

Alternatively, the caller to MockTimers could specify the desired buffer space as a parameter, and return a channel with that capacity. That's almost certainly the easiest solution, it just doesn't work in situations where the test needs to create an unknown number of timers, and can't read them as they're created. Such a situation should probably never occur, however, and if it does, the test could consume from the buffer in a goroutine.

bboozzoo · 2024-10-25T12:37:57Z

testtime/testtime.go

+// accesses of C must be replaced with C().
+//
+// For more information about time.Timer, see: https://pkg.go.dev/time#Timer
+type Timer interface {


I don't think anyone will import a test package to use the interface, but what you could do is simply make sure that the fake timer and stdlib timer match the interface so that both look similar enough in the context of the test code may want to do.

var _ Timer = (*TestTimer)(nil) var _ Timer = (*time.Timer)(nil)

That was the original thought behind the package (see commits prior to e196095), but then production code which uses the C field of time.Timer cannot be replaced by a TestTimer nor the testtime.Timer interface, as interfaces cannot have instance variables.

And even if time.Timer implemented the testtime.Timer interface, in order to have production code which uses time.Timer use a mockable timer instead, wouldn't they have to use testtime.Timer as the type anyway, and just replace time.AfterFunc with testtime.AfterFunc etc.? There's no way to have a variable of type *time.Timer populated with a *testtime.TestTimer

Either the package locally defines a Timer interface using the one here as a reference (i.e. makes a copy), or the Timer interface could be moved to eg. timeutil which other packages can import. Various *test packages are meant to provide things you need for testing only.

Just for the record, we have a single place right now where the Timer interface is used, so I don't mind simply keeping it in the prompting code until we have more opportunities to use it (and the test helpers)

bboozzoo · 2024-10-25T12:39:54Z

testtime/testtime.go

+// time.AfterFunc. See here for more details: https://pkg.go.dev/time#AfterFunc
+func AfterFunc(d time.Duration, f func()) Timer {
+	if useMockedTimers {
+		return afterFuncTest(d, f)


why not always return a fake timer? if I want a fake timer in a test I'll mock relevant code to call testtime.AfterFunc

See my reply above. Before e196095, that's exactly what I used, but then any usage of *time.Timer in production code which leveraged the C field could not be replaced with testtime.Timer, since the latter cannot have a non-method field.

aah, I see. Maye this would work:

package timeutil .. type Timer interface { // Equivalent to t.C for stdlib timer ExpiredC() <-chan time.Time Reset(d time.Duration) bool Stop() bool } type stdlibTimer *time.Timer func (t stdlibTimer) ExpiredC() <-chan time.Time { return t.C } func TimeTimerAdapter(t *time.Timer) Timer { return stdlibTimer(t) }

Ahhhh yes, I think that solves the problems (uses the time.Timer constructors directly, timer.ExpiredC() doesn't conflict with timer.C. Then the only code change needed to replace production code using time.Timer with this would be calling timeutil.TimeTimerAdapter. Nice!

testtime/testtime.go

bboozzoo · 2024-11-04T08:49:28Z

testtime/testtime.go

+func AfterFunc(d time.Duration, f func()) *TestTimer {
+	currTime := time.Now()
+	return &TestTimer{
+		currTime:   currTime,
+		expiration: currTime.Add(d),
+		active:     true,
+		callback:   f,
+	}
+}


with this you should be able to drop the code in run-checks

Suggested change

func AfterFunc(d time.Duration, f func()) *TestTimer {

currTime := time.Now()

return &TestTimer{

currTime: currTime,

expiration: currTime.Add(d),

active: true,

callback: f,

}

}

func AfterFunc(d time.Duration, f func()) *TestTimer {

osutil.MustBeTestBinary("testtime timers cannot be used outside of unit test code")

currTime := time.Now()

return &TestTimer{

currTime: currTime,

expiration: currTime.Add(d),

active: true,

callback: f,

}

}

I'm not sure if this works... If I make this change, plus the ones you suggest, and then run go test from inside client/, the test just hangs (because the timer doesn't advance without manually being advanced):

diff --git a/client/client.go b/client/client.go index d9a936549c..ecb84b83e9 100644 --- a/client/client.go +++ b/client/client.go @@ -38,6 +38,7 @@ import ( "github.com/snapcore/snapd/features" "github.com/snapcore/snapd/httputil" "github.com/snapcore/snapd/jsonutil" + "github.com/snapcore/snapd/testtime" ) func unixDialer(socketPath string) func(string, string) (net.Conn, error) { @@ -391,7 +392,7 @@ func (client *Client) do(method, path string, query url.Values, headers map[stri } retry := time.NewTicker(opts.Retry) defer retry.Stop() - timeout := time.NewTimer(opts.Timeout) + timeout := testtime.NewTimer(opts.Timeout) defer timeout.Stop() for { @@ -409,7 +410,7 @@ func (client *Client) do(method, path string, query url.Values, headers map[stri select { case <-retry.C: continue - case <-timeout.C: + case <-timeout.ExpiredC(): } break }

Ah, I see, anything that imports client then panics immediately, but testing client itself doesn't panic... hmm

I think I'd prefer to leave the static check in place as well as the osutil.MustBeTestBinary() call, since the former can catch usage of testtime.{AfterFunc,NewTimer} even when the user is a package which is not imported elsewhere in the codebase.

testtime/testtime.go

bboozzoo

LGTM

testtime/testtime_test.go

pedronis · 2024-11-07T10:27:00Z

testtime/testtime_test.go

@olivercalder can you also port all the standard libs tests for timers using Elapse where they use Sleep?

To be clear, do you mean the tests from the Go standard library (e.g. https://cs.opensource.google/go/go/+/refs/tags/go1.23.3:src/time/sleep_test.go;l=75) or those from StdlibTimer in timeutil/timer_test.go?

Only the go standard lib tests use sleep, so I'll port those :)

yes, I meant the golang stdlib

pedronis

I think we want to make sure that we can pass similar tests as the stdlib timers

pedronis

bit wondering about the usages of currTime and time.Now here

testtime/testtime.go

olivercalder · 2024-11-08T17:45:19Z

As @pedronis brought up, it could be desirable to have a way of controlling the passage of time for all timers which were created, either by having a testtime.Now which sets the current time for all timers (prior to their redesign to use internal durations/elapsed times instead of currTime/expiration), or a testtime.Elapse which elapses time for all created timers.

As it stands now, users of testtime have to manually extract, keep track of, and elapse time for each created timer individually, which can be tedious and error prone. It would be nice if the testtime package managed that for us, rather than callers doing so.

I worry about namespacing timers which are created during different runs. I think it would be great if there were a TimerFactory which could be created once for each test run, and from which individual timers can be created. Then the factory has methods to elapse time for each timer it owns, and it can be responsible for cleaning up all timers when the test completes (if needed).

pedronis

thank you

changed, needs re-review

bboozzoo

Thanks

olivercalder · 2024-11-12T14:40:36Z

This PR adds a new package which is (currently) unused in other packages and, since it cannot occur in non-test code, by definition cannot be reached in spread tests (other than unit tests).

The current test failures are as follows:

unit-tests-special (1.18, race): FAIL github.com/snapcore/snapd/overlord/snapstate 300.475s --- timeout during snapstate unit tests
opensuse: most tests fail, many related to error: changing owner of fake update dir: exit status 1 or

++ sudo -u '#12345' -g '#12345' ./runas-verify-uidgid
root is not in the sudoers file.
This incident has been reported to the administrator.

google:ubuntu-24.10-64:tests/main/apparmor-prompting-integration-tests:create_multiple_actioned_by_other_pid_always_deny --- timeout installing snap dependencies
google-core:ubuntu-core-16-64:tests/main/apparmor-prompting-integration-tests:write_read_multiple_actioned_by_other_pid_allow_deny --- timeout installing snap dependencies
google-core:ubuntu-core-18-64:tests/core/kernel-base-gadget-pair-single-reboot:kernel_base --- unrelated, it seems a reboot occurred during the test run?
google-core:ubuntu-core-22-64:tests/main/ --- timeout fetching pc-kernel
google-core:ubuntu-core-24-64:tests/main/ --- timeout running snap pack --filename=core24-repacked.snap core24-snap
google-pro:ubuntu-fips-22.04-64:tests/fips/ --- unrelated

I think this PR can be force-merged, which will make review of #14390 easier.

zyga

I have one question about run-checks.

I don't particularly love it, can we do what we did with osuser changes recently?

zyga · 2024-11-08T09:03:09Z

run-checks

@@ -241,6 +241,21 @@ if [ "$STATIC" = 1 ]; then
        exit 1
    fi

+    echo "Checking for usages of testtime outside of test code"
+    got=""


Can we use one of the linters instead? We did that for osuser changes.

For some reason it seems my golangci-lint is not catching the new lint I'm trying to add, nor the existing lint for osuser. I'm using client/client.go here as a testing ground to verify whether the lints catch package misuse:

diff --git a/.golangci.yml b/.golangci.yml index 71a0cecb78..fef62554f0 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -129,6 +129,14 @@ linters-settings: deny: - pkg: "os/user" desc: "Please use osutil/user instead. See https://github.com/canonical/snapd/pull/13776" + testtime: + files: + - "!$test" + deny: + - pkg: "github.com/snapcore/snapd/testtime" + desc: "Cannot use testtime outside of test code" + - pkg: "github.com/canonical/snapd/testtime" + desc: "Cannot use testtime outside of test code" misspell: # Correct spellings using locale preferences for US or UK. diff --git a/client/client.go b/client/client.go index d9a936549c..7c054cb191 100644 --- a/client/client.go +++ b/client/client.go @@ -30,6 +30,7 @@ import ( "net/http" "net/url" "os" + "os/user" "path" "strconv" "time" @@ -38,9 +39,11 @@ import ( "github.com/snapcore/snapd/features" "github.com/snapcore/snapd/httputil" "github.com/snapcore/snapd/jsonutil" + "github.com/snapcore/snapd/testtime" ) func unixDialer(socketPath string) func(string, string) (net.Conn, error) { + user.Current() if socketPath == "" { socketPath = dirs.SnapdSocket } @@ -391,7 +394,7 @@ func (client *Client) do(method, path string, query url.Values, headers map[stri } retry := time.NewTicker(opts.Retry) defer retry.Stop() - timeout := time.NewTimer(opts.Timeout) + timeout := testtime.NewTimer(opts.Timeout) defer timeout.Stop() for { @@ -409,7 +412,7 @@ func (client *Client) do(method, path string, query url.Values, headers map[stri select { case <-retry.C: continue - case <-timeout.C: + case <-timeout.ExpiredC(): } break }

When I run golangci-lint run from the project root, it doesn't catch anything wrong.

Interestingly, when I run golangci-lint linters, it shows depguard under the list of Disabled by your configuration linters.

Running with golangci-lint run --config .golangci.yml also does not catch the expected lint errors.

We're missing depguard in enabled linters. Let me open a quick PR for that.

See #14717

With depguard enabled, an intentional use of os/user is flagged:

maciek@galeon:work/canonical/snapd (git)-[bboozzoo/enable-depguard] golangci-lint run cmd/snap/main.go:28:2: import 'os/user' is not allowed from list 'osuser': Please use osutil/user instead. See https://github.com/canonical/snapd/pull/13776 (depguard) "os/user" ^

Signed-off-by: Oliver Calder <[email protected]>

Rather than returning whether the timer had fired, `Stop` and `Reset` should return whether the timer is currently active. If it has fired or been stopped manually, these should return false. Additionally, add the `Active` method to report whether the timer is currently active, and add the `FireCount` method to report the number of times that the timer has fired. This is useful to directly check whether a timer's callback has been called. Signed-off-by: Oliver Calder <[email protected]>

Signed-off-by: Oliver Calder <[email protected]>

The `time.Timer` type uses an instance variable `C` for the channel over which the expiration time is sent when a timer which was created via `time.NewTimer` fires. Interfaces in Go cannot have instance variables, so we must use a `C()` method instead. However, this means `time.Timer` cannot implement our `testtime.Timer` interface fully. Thus, this commit adds `testtime.RealTimer` as a wrapper around `time.Timer` which exposes the latter's inner `C` variable as `C()`. Since we now need to construct `testtime.RealTimer` instances in place of `time.Timer` instances in production code, the `testtime.AfterFunc` and `testtime.NewTimer` functions now return `testtime.RealTimer`s by default, which are thin wrappers around `time.Timer`s. In test code, tests can call `testtime.MockTimers` to make all subsequent invocations of `testtime.AfterFunc` and `testtime.NewTimer` return `testtime.TestTimer`s instead of `testtime.RealTimer`s. It is important that `testtime.MockTimers` is never used in non-test code, so this commit adds a static check to ensure this is the case. Signed-off-by: Oliver Calder <[email protected]>

… timer Signed-off-by: Oliver Calder <[email protected]>

Signed-off-by: Oliver Calder <[email protected]>

…tover test Signed-off-by: Oliver Calder <[email protected]>

Signed-off-by: Oliver Calder <[email protected]>

… test helper Signed-off-by: Oliver Calder <[email protected]>

Signed-off-by: Oliver Calder <[email protected]>

Since `TestTimer` is designed to allow the passage of time to be controlled manually, it makes sense to represent this internally using a duration and elapsed time, rather than storing the timestamp at the creation of the timer and advancing an internal "current time". This way, the time at which the timer was created has no effect on the expiration timestamp which may be sent over the C channel when it fires, and timers don't each have their own conflicting view of what time it is. When a timer expires, the current time is sent over the channel, or if `Fire` is called directly, then the given time is sent instead. Signed-off-by: Oliver Calder <[email protected]>

Signed-off-by: Oliver Calder <[email protected]>

zyga

Woot, thank you so much for doing this.

I hope that we can slowly reduce the number of tests that rely on <100ms sleeps mocked via timing hacks.

* master: (44 commits) wrappers: do not reload activation units (canonical#14724) gadget/install: support for no{exec,dev,suid} mount flags interfaces/builtin/mount_control: add support for nfs mounts (canonical#14694) tests: use gojq - part 1 (canonical#14686) interfaces/desktop-legacy: allow DBus access to com.canonical.dbusmenu interfaces/builtin/fwupd.go: allow access to nvmem for thunderbolt plugin tests: removing uc16 executions (canonical#14575) tests: Added arm github runner to build snapd (canonical#14504) tests: no need to run spread when there are not tests matching the filter (canonical#14728) tests/lib/tools/store-state: exit on errors, update relevant tests (canonical#14725) tests: udpate the github workflow to run tests suggested by spread-filter tool (canonical#14519) testtime: add mockable timers for use in tests (canonical#14672) interface/screen_inhibit_control: Improve screen inhibit control for use on core (canonical#14134) tests: use images with 20G disk in openstack (canonical#14720) i/builtin: allow @ in custom-device filepaths (canonical#14651) tests: refactor test-snapd-desktop-layout-with-content tests: fix broken app definition tests: capitalize sentences in comments tests: wrap very long shell line tests: fix raciness in async startup and sync install ...

olivercalder added the Needs Samuele review Needs a review from Samuele before it can land label Oct 24, 2024

olivercalder requested review from bboozzoo, zyga and pedronis October 24, 2024 21:55

olivercalder mentioned this pull request Oct 24, 2024

i/p/requestprompts: add timeout for prompts #14390

Open

bboozzoo reviewed Oct 25, 2024

View reviewed changes

bboozzoo reviewed Nov 4, 2024

View reviewed changes

bboozzoo previously approved these changes Nov 7, 2024

View reviewed changes

testtime/testtime_test.go Show resolved Hide resolved

pedronis reviewed Nov 7, 2024

View reviewed changes

pedronis requested changes Nov 7, 2024

View reviewed changes

olivercalder requested a review from pedronis November 8, 2024 05:13

pedronis reviewed Nov 8, 2024

View reviewed changes

testtime/testtime.go Outdated Show resolved Hide resolved

testtime/testtime.go Outdated Show resolved Hide resolved

testtime/testtime.go Outdated Show resolved Hide resolved

testtime/testtime.go Outdated Show resolved Hide resolved

testtime/testtime.go Outdated Show resolved Hide resolved

olivercalder requested a review from pedronis November 8, 2024 17:36

pedronis approved these changes Nov 8, 2024

View reviewed changes

olivercalder requested a review from bboozzoo November 8, 2024 19:26

bboozzoo approved these changes Nov 12, 2024

View reviewed changes

zyga reviewed Nov 12, 2024

View reviewed changes

olivercalder added 8 commits November 13, 2024 08:54

timeutil: add timer interface and fake timer

529b673

Signed-off-by: Oliver Calder <[email protected]>

testtime,timeutil: move mockable timer to dedicated testtime package

570b62c

Signed-off-by: Oliver Calder <[email protected]>

testtime,timeutil: move timer to timeutil and use testtime for mocked…

a91660b

… timer Signed-off-by: Oliver Calder <[email protected]>

testtime: panic if used in non-test code or fire called when inactive

4394bfd

Signed-off-by: Oliver Calder <[email protected]>

testtime: add checks that firing inactive timer panics and remove lef…

4252ea5

…tover test Signed-off-by: Oliver Calder <[email protected]>

randutil: export Perm function from math/rand

28649eb

Signed-off-by: Oliver Calder <[email protected]>

olivercalder added 6 commits November 13, 2024 08:54

testtime: add timer tests from the go standard library

24aaf63

Signed-off-by: Oliver Calder <[email protected]>

testtime: prefer callback to expired channel and improve newTimerFunc…

890b1fc

… test helper Signed-off-by: Oliver Calder <[email protected]>

testtime: rename internal lock to mu

73d8ecc

Signed-off-by: Oliver Calder <[email protected]>

.golangci.yml: add lint to check for usage of testtime in non-test code

49f9098

Signed-off-by: Oliver Calder <[email protected]>

run-checks: remove manual check for testtime in favor of lint

1612bba

Signed-off-by: Oliver Calder <[email protected]>

olivercalder force-pushed the add-mockable-timer-for-tests branch from 8eaf3c2 to 1612bba Compare November 13, 2024 14:54

olivercalder requested a review from zyga November 13, 2024 17:29

zyga approved these changes Nov 14, 2024

View reviewed changes

olivercalder merged commit e716737 into canonical:master Nov 14, 2024
55 of 57 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

testtime: add mockable timers for use in tests #14672

testtime: add mockable timers for use in tests #14672

olivercalder commented Oct 24, 2024 •

edited

Loading

codecov bot commented Oct 24, 2024 •

edited

Loading

olivercalder commented Oct 24, 2024

bboozzoo Oct 25, 2024

olivercalder Oct 25, 2024

bboozzoo Oct 25, 2024

bboozzoo Oct 25, 2024

olivercalder Oct 25, 2024

bboozzoo Oct 25, 2024

olivercalder Oct 25, 2024

bboozzoo Nov 4, 2024

olivercalder Nov 6, 2024 •

edited

Loading

olivercalder Nov 6, 2024

olivercalder Nov 6, 2024

bboozzoo left a comment

pedronis Nov 7, 2024 •

edited

Loading

olivercalder Nov 7, 2024 •

edited

Loading

pedronis Nov 7, 2024

pedronis left a comment

pedronis left a comment

olivercalder commented Nov 8, 2024

pedronis left a comment

bboozzoo left a comment

olivercalder commented Nov 12, 2024

zyga left a comment

zyga Nov 8, 2024

olivercalder Nov 12, 2024 •

edited

Loading

olivercalder Nov 12, 2024

olivercalder Nov 12, 2024

bboozzoo Nov 13, 2024

bboozzoo Nov 13, 2024

zyga left a comment

testtime: add mockable timers for use in tests #14672

testtime: add mockable timers for use in tests #14672

Conversation

olivercalder commented Oct 24, 2024 • edited Loading

codecov bot commented Oct 24, 2024 • edited Loading

Codecov Report

olivercalder commented Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivercalder Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo left a comment

Choose a reason for hiding this comment

pedronis Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

olivercalder Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

olivercalder commented Nov 8, 2024

pedronis left a comment

Choose a reason for hiding this comment

bboozzoo left a comment

Choose a reason for hiding this comment

olivercalder commented Nov 12, 2024

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivercalder Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

olivercalder commented Oct 24, 2024 •

edited

Loading

codecov bot commented Oct 24, 2024 •

edited

Loading

olivercalder Nov 6, 2024 •

edited

Loading

pedronis Nov 7, 2024 •

edited

Loading

olivercalder Nov 7, 2024 •

edited

Loading

olivercalder Nov 12, 2024 •

edited

Loading