Add Clock Abstraction for Deterministic Testing

[Satyam709]

Add Clock Abstraction for Deterministic Testing

Introduces a new Clock API for deterministic logic testing and validation.

---

Problem

Several endpoints and calculations in the codebase are time-dependent e.g. active stops filtering, arrivals/departures, schedule lookups, trip details, etc.

The challenge comes in testing this logic deterministically. Tests that rely on GTFS schedule data behave differently depending on when they run passing on weekdays but failing on weekends or holidays when transit services don't operate.

Example

TestStopsForLocationHandlerEndToEnd fails on several days (e.g., Dec 7, 14, 21, 25) while passing on others. The test queries stops near a location, but stops are filtered based on whether they have active service on the current date:

// internal/gtfs/gtfs_manager.go
if !queryTime.IsZero() {
    currentDate = queryTime.Format("20060102")
} else {
    currentDate = time.Now().Format("20060102")  // ← uses system time
}

activeServiceIDs, _ := manager.GtfsDB.Queries.GetActiveServiceIDsForDate(ctx, currentDate)

This PR fixes this for TestStopsForLocationEndToEnd and provides the foundation to fix similar issues across the codebase.

---

Solution

Clock Interface

type Clock interface {
    Now() time.Time
    NowUnixMilli() int64
}

Implementations

Type	Purpose
RealClock	Production — uses time.Now()
MockClock	Unit tests — returns fixed/controlled time
EnvironmentClock	Integration tests — reads time from env var or file

---

Usage

RealClock (Production)

import "maglev.onebusaway.org/internal/clock"

// Default for production use
c := clock.RealClock{}
now := c.Now()           // Returns time.Now()
millis := c.NowUnixMilli() // Returns current time in milliseconds

MockClock (Unit Tests)

import "maglev.onebusaway.org/internal/clock"

// Create with a fixed time
fixedTime := time.Date(2025, 12, 26, 14, 39, 18, 0, time.UTC)
c := clock.NewMockClock(fixedTime)

// Get the current (fixed) time
now := c.Now()  // Always returns 2025-12-26 14:39:18

// Change the time
c.Set(time.Date(2025, 6, 15, 10, 0, 0, 0, time.UTC))

// Advance time by duration
c.Advance(1 * time.Hour)      // Move forward 1 hour
c.Advance(-30 * time.Minute)  // Move backward 30 minutes

Test Example:

func TestStopsForLocationHandlerEndToEnd(t *testing.T) {
    // Fixed date ensures stop is active
    clock := clock.NewMockClock(time.Date(2025, 12, 26, 14, 39, 18, 0, time.UTC))
    api := createTestApiWithClock(t, clock)
    
    resp, model := serveApiAndRetrieveEndpoint(t, api, 
        "/api/where/stops-for-location.json?key=TEST&lat=40.583321&lon=-122.426966")
    
    assert.Equal(t, http.StatusOK, resp.StatusCode)
}

EnvironmentClock (Integration Tests)

Reads time from external sources, useful for integration tests where time can be controlled by environment without code changes.

import "maglev.onebusaway.org/internal/clock"

// Priority: env var → file → system time (fallback)
c := clock.NewEnvironmentClock(
    "MAGLEV_TIME",              // Environment variable name
    "/tmp/maglev-time.txt",     // File path
    time.UTC,                   // Location for parsing
)

now := c.Now()  // Reads from configured sources

In Handlers

func (api *Application) stopsForLocationHandler(w http.ResponseWriter, r *http.Request) {
    // Use injected clock instead of time.Now()
    queryTime := api.Clock.Now()
    
    stops := api.GtfsManager.GetStopsForLocation(ctx, lat, lon, radius, 
        latSpan, lonSpan, query, maxCount, false, routeTypes, queryTime)
}

In Response Models

// All response builders accept Clock for consistent currentTime field
response := models.NewListResponseWithRange(results, references, true, api.Clock)
response := models.NewEntryResponse(entry, references, api.Clock)
response := models.NewOKResponse(data, api.Clock)

Advantages

• Enables reliable testing of GTFS service calendars, schedule boundaries, and date-specific logic by making time an explicit dependency.
• Improves architectural clarity by removing implicit reliance on time.Now() and centralizing time access behind a well-defined abstraction.
• Provides controlled time manipulation in unit and integration tests without impacting production behavior, and can be extended to support api-validator tool for more exhaustive integration testing.

[Satyam709]

@aaronbrethorst could you review this PR and share your thoughts

[aaronbrethorst]

@Satyam709 will do, this is really cool!

[Satyam709]

I’ve fixed a few more tests failing for the same reason
I think this should be good to merge for now, and the remaining tests can be fixed going forward.
Happy to make further changes if needed.

[aaronbrethorst]

Thanks for putting this together! This is a well-motivated PR that addresses a real pain point with flaky tests caused by time-dependent logic. The Clock interface design is clean, the documentation in the PR description is thorough, and the test coverage for the clock package is comprehensive.

I found a few issues we'll need to address before merging.

---

Issues to Fix

1. MockClock is not thread-safe

In internal/clock/clock.go, the MockClock struct has a public CurrentTime field and the Set/Advance methods don't use any synchronization:

type MockClock struct {
	CurrentTime time.Time
}

func (m *MockClock) Set(t time.Time) {
	m.CurrentTime = t
}

func (m *MockClock) Advance(d time.Duration) {
	m.CurrentTime = m.CurrentTime.Add(d)
}

What needs to change:

• If MockClock is used in concurrent tests (e.g., parallel subtests), this could cause data races
• Consider either adding a mutex, or documenting that MockClock is not safe for concurrent use
• Since the field is public, users could also access it directly causing races with Now()

---

2. Custom trimWhitespace instead of strings.TrimSpace

In internal/clock/clock.go, there's a custom implementation:

func trimWhitespace(s string) string {
	// Simple trim for newlines and spaces
	for len(s) > 0 && (s[0] == ' ' || s[0] == '\t' || s[0] == '\n' || s[0] == '\r') {
		s = s[1:]
	}
	// ...
}

What needs to change:

• Replace with strings.TrimSpace(s) which is more robust and handles all Unicode whitespace
• This reduces code to maintain and follows Go conventions

---

3. Potential semantic change in routes_for_location_handler.go

In internal/restapi/routes_for_location_handler.go, the behavior has changed:

// Before:
stops := api.GtfsManager.GetStopsForLocation(ctx, ..., time.Time{})

// After:
stops := api.GtfsManager.GetStopsForLocation(ctx, ..., api.Clock.Now())

What needs to change:

• Verify this is intentional - passing a zero time.Time{} vs actual current time may have different semantics in GetStopsForLocation
• If the zero value meant "don't filter by time", this could change behavior
• If intentional, this is fine but worth confirming

---

4. EnvironmentClock mutex may not be necessary

In internal/clock/clock.go, the mutex protects os.Getenv and os.ReadFile:

func (e *EnvironmentClock) syncFromEnvVar() (time.Time, error) {
	e.mutex.Lock()
	defer e.mutex.Unlock()
	// os.Getenv is already thread-safe
	timeStr := os.Getenv(e.envVar)
	// ...
}

Suggestion:

• os.Getenv and os.ReadFile are already thread-safe
• The mutex could be removed unless there's a specific reason for serializing calls
• If keeping it, consider whether holding the lock during file I/O could cause unnecessary contention

---

5. EnvironmentClock doesn't handle nil location

In internal/clock/clock.go, if location is nil, parseTime will panic:

func (e *EnvironmentClock) parseTime(s string) (time.Time, error) {
	// ...
	if t, err := time.ParseInLocation(format, s, e.location); err == nil {  // panics if e.location is nil
		return t, nil
	}
}

What needs to change:

• Either document that location must not be nil, or
• Default to time.Local or time.UTC if nil is passed

---

Suggestions (Non-blocking)

Consider making MockClock.CurrentTime private

Making the field private and only exposing it via methods would give you more control over thread-safety and prevent direct field access:

type MockClock struct {
	currentTime time.Time
}

Add a test verifying the specific date fix

Since the PR specifically mentions fixing TestStopsForLocationHandlerEndToEnd failing on Dec 7, 14, 21, 25, consider adding a comment in the test explaining why the chosen date (2025-12-26) was selected, to help future maintainers understand the context.

---

This is a valuable addition to the codebase! The core design is solid. Once the thread-safety and minor issues are addressed, this will be ready to merge. The change to enable deterministic testing is well worth the refactoring effort across the handlers.