Summary
Introduce a Clock seam into acquire_with_octocrab_installation (in src/github/installation_token.rs) so that token acquisition timing is deterministically testable without mocking wall-clock time, and thread an explicit metrics::Recorder reference through the acquisition path to eliminate implicit global-state side effects.
Motivation
src/github/installation_token.rs currently calls Instant::now() and SystemTime::now() directly, and emits metrics via the implicit global metrics recorder. This means:
- Non-deterministic timing — tests cannot control the elapsed duration reported in latency histograms or log fields, so assertions on timing values are inherently flaky or must be left out.
- Implicit global-state mutation —
metrics::counter! and metrics::histogram! write to whatever recorder happens to be installed globally at the time. This makes isolated unit tests harder and can cause interference between parallel test threads.
Acceptance criteria
- Define or reuse a
Clock trait (e.g. mockable::Clock or an equivalent in this crate) with at minimum a now() -> Instant (or equivalent) method.
- Inject an
&impl Clock parameter into acquire_with_octocrab_installation (and any private helpers that call Instant::now() / SystemTime::now()).
- Inject an explicit
&dyn metrics::Recorder (or &impl metrics::Recorder) parameter so tests can pass RecordingMetrics without installing a global recorder.
- Provide a
SystemClock (production) and a FixedClock (test) implementation.
- Add deterministic unit tests for the latency histogram and timeout-failure counter using
FixedClock and RecordingMetrics.
- Remove or replace direct calls to
Instant::now() and SystemTime::now() within the acquisition path.
References
Summary
Introduce a
Clockseam intoacquire_with_octocrab_installation(insrc/github/installation_token.rs) so that token acquisition timing is deterministically testable without mocking wall-clock time, and thread an explicitmetrics::Recorderreference through the acquisition path to eliminate implicit global-state side effects.Motivation
src/github/installation_token.rscurrently callsInstant::now()andSystemTime::now()directly, and emits metrics via the implicit globalmetricsrecorder. This means:metrics::counter!andmetrics::histogram!write to whatever recorder happens to be installed globally at the time. This makes isolated unit tests harder and can cause interference between parallel test threads.Acceptance criteria
Clocktrait (e.g.mockable::Clockor an equivalent in this crate) with at minimum anow() -> Instant(or equivalent) method.&impl Clockparameter intoacquire_with_octocrab_installation(and any private helpers that callInstant::now()/SystemTime::now()).&dyn metrics::Recorder(or&impl metrics::Recorder) parameter so tests can passRecordingMetricswithout installing a global recorder.SystemClock(production) and aFixedClock(test) implementation.FixedClockandRecordingMetrics.Instant::now()andSystemTime::now()within the acquisition path.References
src/github/installation_token.rs,src/github/test_support.rs