keys.go (2)

71-75: Consider enhancing error message for debugging.

The implementation correctly decodes the binary format and ensures UTC timezone consistency. However, the error message could be more helpful for debugging.

Consider enhancing the panic message:

-		panic("invalid time key")
+		panic(fmt.Errorf("invalid time key: expected 8 bytes, got %d bytes %s", len(b), HumanizeBytes(b)))

---

63-75: Document breaking changes and provide migration guidance.

This change in time key encoding is a breaking change that affects data persistence. To ensure a smooth upgrade path:

1. Document the breaking change in CHANGELOG.md
2. Provide migration scripts or guidance for users to re-encode existing time-based keys
3. Consider adding version bytes to the key encoding to support future changes

Would you like assistance in drafting the migration documentation or implementing a migration helper function?

keyset_test.go (4)

108-108: Simplify time comparisons using time.Equal

When comparing time.Time values, you can use the built-in Equal method instead of stripping monotonic clock readings and comparing UnixNano timestamps. This simplifies the code and improves readability.

Apply this refactor to simplify time comparisons:

-expectedTime := times[i].Round(0)
-actualTime := k.Round(0)
-require.Equal(t, expectedTime.UnixNano(), actualTime.UnixNano())
+require.True(t, times[i].Equal(k))

Repeat this change in all test functions where time comparisons are performed.

Also applies to: 144-144, 154-154, 192-192, 213-213, 251-251

---

220-227: Extend tests with sub-second time differences

The current tests use time differences in whole seconds. To thoroughly verify the lexicographical order preservation, consider adding times with sub-second differences. This ensures the encoding handles times with finer granularity.

Extend the times slice with sub-second time values:

 times := []time.Time{
 	now.Add(1 * time.Second),
 	now.Add(2 * time.Second),
 	now.Add(3 * time.Second),
 	now.Add(4 * time.Second),
 	now.Add(5 * time.Second),
+	now.Add(5*time.Second + 500*time.Millisecond),
+	now.Add(5*time.Second + 750*time.Millisecond),
 }

---

255-275: Test insertion of times with identical wall times but different monotonic clocks

Currently, the test inserts the exact same time.Time value multiple times. Consider testing with times that have the same wall clock time but different monotonic clock readings to ensure that they are treated as equal keys in the KeySet.

Modify the test to create times with identical wall times:

 now := time.Date(2021, time.January, 1, 0, 0, 0, 0, time.UTC)

+nowWithDifferentMonotonic := now.Add(0 * time.Nanosecond)
+
 // Insert the same time multiple times (should only be stored once in a set)
 keyset.Insert(ctx, now)
+keyset.Insert(ctx, nowWithDifferentMonotonic)
 keyset.Insert(ctx, now)
 keyset.Insert(ctx, now)

Verify that only one key exists in the set:

 require.Equal(t, 1, len(keys))
+require.True(t, keys[0].Equal(now))
+require.True(t, keys[0].Equal(nowWithDifferentMonotonic))

---

149-155: Ensure encoding and decoding handles time zones correctly

In TestTimeKeyEncoder_EncodeDecode, consider adding checks to verify that the encoding and decoding process correctly handles time.Time values with different locations (time zones).

Extend the test to include times with different locations:

 now := time.Date(2021, time.January, 1, 0, 0, 0, 0, time.UTC)
+loc, _ := time.LoadLocation("America/New_York")
+nowNY := now.In(loc)

 encoded := TimeKeyEncoder.Encode(now)
 _, decoded := TimeKeyEncoder.Decode(encoded)

+encodedNY := TimeKeyEncoder.Encode(nowNY)
+_, decodedNY := TimeKeyEncoder.Decode(encodedNY)

 // Compare UnixNano timestamps
 require.Equal(t, now.UnixNano(), decoded.UnixNano())
+require.Equal(t, nowNY.UnixNano(), decodedNY.UnixNano())

Configuration used: CodeRabbit UI
Review profile: CHILL