Skip to content

Commit 4fc97a0

Browse files
committed
fix(agents): correct package names and clock API in skills (#1697)
- Fix MySQL test commands: replace torrust-tracker-core / tracker-core with the correct Cargo package name bittorrent-tracker-core in packages/AGENTS.md and run-pre-commit-checks/SKILL.md - Fix write-unit-test/SKILL.md: replace fictional Arc<dyn Clock> injection pattern and MockClock::new() constructor with the real type-level CurrentClock alias (clock::Working / clock::Stopped) and Stopped::local_set() for deterministic time in tests; also fix cargo test -p tracker-core -> bittorrent-tracker-core and update quick checklist
1 parent 492a8ac commit 4fc97a0

3 files changed

Lines changed: 52 additions & 31 deletions

File tree

.github/skills/dev/git-workflow/run-pre-commit-checks/SKILL.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ The script runs these steps in order:
4343
> **MySQL tests**: MySQL-specific tests require a running instance and a feature flag:
4444
>
4545
> ```bash
46-
> TORRUST_TRACKER_CORE_RUN_MYSQL_DRIVER_TEST=true cargo test --package tracker-core
46+
> TORRUST_TRACKER_CORE_RUN_MYSQL_DRIVER_TEST=true cargo test --package bittorrent-tracker-core
4747
> ```
4848
>
4949
> These are not run by the pre-commit script.

.github/skills/dev/testing/write-unit-test/SKILL.md

Lines changed: 50 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ mod tests {
9090

9191
```bash
9292
# Run all tests in a package
93-
cargo test -p tracker-core
93+
cargo test -p bittorrent-tracker-core
9494

9595
# Run specific test by name
9696
cargo test it_should_return_error_when_info_hash_is_invalid
@@ -102,61 +102,82 @@ cargo test info_hash::tests
102102
cargo test -- --nocapture
103103
```
104104

105-
## Phase 2: Deterministic Time with MockClock
105+
## Phase 2: Deterministic Time with `clock::Stopped`
106106

107-
The `clock` workspace package provides a `MockClock` for deterministic time testing.
108-
Never use `std::time::SystemTime::now()` or `chrono::Utc::now()` directly in production code
109-
that needs testing.
107+
The `clock` workspace package provides `clock::Stopped` for deterministic time testing.
108+
Never call `std::time::SystemTime::now()` or `chrono::Utc::now()` directly in production code
109+
that needs testing. Instead, use the type-level clock abstraction.
110110

111-
### Inject the Clock Dependency
111+
### Use the Type-Level Clock Alias
112+
113+
Copy the following boilerplate into each crate that needs a clock. The `CurrentClock` alias
114+
automatically selects `Working` in production and `Stopped` in tests:
112115

113116
```rust
114-
use torrust_tracker_clock::clock::Clock;
115-
use std::sync::Arc;
117+
/// Working version, for production.
118+
#[cfg(not(test))]
119+
pub(crate) type CurrentClock = torrust_tracker_clock::clock::Working;
116120

117-
pub struct PeerList {
118-
clock: Arc<dyn Clock>,
119-
}
121+
/// Stopped version, for testing.
122+
#[cfg(test)]
123+
pub(crate) type CurrentClock = torrust_tracker_clock::clock::Stopped;
124+
```
120125

121-
impl PeerList {
122-
pub fn new(clock: Arc<dyn Clock>) -> Self {
123-
Self { clock }
124-
}
126+
In production code, obtain the current time via the `Time` trait:
125127

126-
pub fn is_peer_expired(&self, last_seen: i64, ttl: u32) -> bool {
127-
let now = self.clock.now();
128-
now - last_seen > i64::from(ttl)
129-
}
128+
```rust
129+
use torrust_tracker_clock::clock::Time as _;
130+
131+
pub fn is_peer_expired(last_seen: std::time::Duration, ttl: u32) -> bool {
132+
let now = CurrentClock::now(); // returns DurationSinceUnixEpoch (= std::time::Duration)
133+
now.saturating_sub(last_seen) > std::time::Duration::from_secs(u64::from(ttl))
130134
}
131135
```
132136

133-
### Use MockClock in Tests
137+
### Control Time in Tests
138+
139+
Use `clock::Stopped::local_set` to pin the clock to a specific instant. The stopped clock is
140+
thread-local, so tests are isolated from each other by default.
134141

135142
```rust
136143
#[cfg(test)]
137144
mod tests {
145+
use std::time::Duration;
146+
147+
use torrust_tracker_clock::clock::{stopped::Stopped as _, Time as _};
148+
use torrust_tracker_clock::clock::Stopped;
149+
138150
use super::*;
139-
use torrust_tracker_clock::clock::stopped::Stopped as MockClock;
140-
use std::sync::Arc;
141151

142152
#[test]
143153
fn it_should_mark_peer_as_expired_when_ttl_has_elapsed() {
144-
// Arrange
145-
let fixed_time = 1_700_000_100i64; // specific Unix timestamp
146-
let clock = Arc::new(MockClock::new(fixed_time));
147-
let list = PeerList::new(clock);
148-
let last_seen = 1_700_000_000i64;
154+
// Arrange — pin the clock to a known instant
155+
let fixed_time = Duration::from_secs(1_700_000_100);
156+
Stopped::local_set(&fixed_time);
157+
158+
let last_seen = Duration::from_secs(1_700_000_000);
149159
let ttl = 60u32;
150160

151161
// Act
152-
let expired = list.is_peer_expired(last_seen, ttl);
162+
let expired = is_peer_expired(last_seen, ttl);
153163

154164
// Assert
155165
assert!(expired);
166+
167+
// Clean up — reset to zero so other tests start from a clean state
168+
Stopped::local_reset();
156169
}
157170
}
158171
```
159172

173+
> **Key points**
174+
>
175+
> - `Stopped::now()` defaults to `Duration::ZERO` at the start of each test thread.
176+
> - `Stopped::local_set(&duration)` sets the current time for the calling thread only.
177+
> - `Stopped::local_reset()` resets back to `Duration::ZERO`.
178+
> - `Stopped::local_add(&duration)` advances the clock by the given amount.
179+
> - Import the `Stopped` trait (`use …::stopped::Stopped as _`) to bring its methods into scope.
180+
160181
## Phase 3: Parameterized Tests with rstest
161182

162183
Use `rstest` for multiple input/output combinations to avoid repetition.
@@ -195,6 +216,6 @@ Check the package for available mock servers, fixture generators, and utility ty
195216

196217
- [ ] Test name uses `it_should_` prefix
197218
- [ ] Test follows AAA pattern with comments (`// Arrange`, `// Act`, `// Assert`)
198-
- [ ] No `std::time::SystemTime::now()` in production code — inject `Clock` instead
219+
- [ ] No `std::time::SystemTime::now()` in production code — use the `CurrentClock` type alias instead
199220
- [ ] No shared mutable state between tests
200221
- [ ] `cargo test -p <package>` passes

packages/AGENTS.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ cargo test -p <package-name>
134134
cargo test --doc -p <package-name>
135135

136136
# MySQL-specific tests in tracker-core (requires a running MySQL instance)
137-
TORRUST_TRACKER_CORE_RUN_MYSQL_DRIVER_TEST=true cargo test -p torrust-tracker-core
137+
TORRUST_TRACKER_CORE_RUN_MYSQL_DRIVER_TEST=true cargo test -p bittorrent-tracker-core
138138
```
139139

140140
Use `clock::Stopped` (from the `clock` package) in unit tests that need deterministic time.

0 commit comments

Comments
 (0)