-
Notifications
You must be signed in to change notification settings - Fork 19
fix: isRegistrationFullyExpired helper
#2348
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
|
|
||
| import { | ||
| isRegistrationExpired, | ||
| isRegistrationFullyExpired, | ||
| isRegistrationInGracePeriod, | ||
| } from "./registration-expiration"; | ||
|
|
||
| describe("registration expiration", () => { | ||
| const expiry = 1000n; | ||
| const gracePeriod = 100n; | ||
|
|
||
| describe("isRegistrationExpired", () => { | ||
| it.each([ | ||
| { now: 999n, expected: false, description: "before expiry" }, | ||
| { now: 1000n, expected: true, description: "at expiry" }, | ||
| { now: 1001n, expected: true, description: "after expiry" }, | ||
| ])("returns $expected when $description", ({ now, expected }) => { | ||
| expect(isRegistrationExpired({ expiry, gracePeriod }, now)).toBe(expected); | ||
| }); | ||
|
|
||
| it("returns false when expiry is null", () => { | ||
| expect(isRegistrationExpired({ expiry: null, gracePeriod }, 2000n)).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe("isRegistrationFullyExpired", () => { | ||
| it.each([ | ||
| { now: 999n, expected: false, description: "before expiry" }, | ||
| { now: 1000n, expected: false, description: "at expiry" }, | ||
| { now: 1050n, expected: false, description: "during grace period" }, | ||
| { now: 1100n, expected: false, description: "at expiry + grace period" }, | ||
| { now: 1101n, expected: true, description: "after expiry + grace period" }, | ||
| ])("returns $expected when $description", ({ now, expected }) => { | ||
| expect(isRegistrationFullyExpired({ expiry, gracePeriod }, now)).toBe(expected); | ||
| }); | ||
|
|
||
| it("returns false when expiry is null", () => { | ||
| expect(isRegistrationFullyExpired({ expiry: null, gracePeriod }, 2000n)).toBe(false); | ||
| }); | ||
|
|
||
| it("treats null grace period as zero", () => { | ||
| expect(isRegistrationFullyExpired({ expiry, gracePeriod: null }, 1000n)).toBe(false); | ||
| expect(isRegistrationFullyExpired({ expiry, gracePeriod: null }, 1001n)).toBe(true); | ||
| }); | ||
| }); | ||
|
|
||
| describe("isRegistrationInGracePeriod", () => { | ||
| it.each([ | ||
| { now: 999n, expected: false, description: "before expiry" }, | ||
| { now: 1000n, expected: true, description: "at expiry" }, | ||
| { now: 1050n, expected: true, description: "during grace period" }, | ||
| { now: 1100n, expected: false, description: "at expiry + grace period" }, | ||
| { now: 1101n, expected: false, description: "after expiry + grace period" }, | ||
| ])("returns $expected when $description", ({ now, expected }) => { | ||
| expect(isRegistrationInGracePeriod({ expiry, gracePeriod }, now)).toBe(expected); | ||
| }); | ||
|
|
||
| it("returns false when expiry is null", () => { | ||
| expect(isRegistrationInGracePeriod({ expiry: null, gracePeriod }, 1050n)).toBe(false); | ||
| }); | ||
|
|
||
| it("returns false when grace period is null", () => { | ||
| expect(isRegistrationInGracePeriod({ expiry, gracePeriod: null }, 1050n)).toBe(false); | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,8 +23,8 @@ export function isRegistrationFullyExpired(info: RegistrationExpiryInfo, now: bi | |
| // no expiry, never expired | ||
| if (info.expiry == null) return false; | ||
|
|
||
| // otherwise it is expired if now >= expiry + grace | ||
| return now >= info.expiry + (info.gracePeriod ?? 0n); | ||
| // otherwise it is expired if now > expiry + grace | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| return now > info.expiry + (info.gracePeriod ?? 0n); | ||
|
Comment on lines
+26
to
+27
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
After the fix, at exactly The original |
||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test asserts that
isRegistrationFullyExpiredreturnsfalsewhennow == expiry + gracePeriod(1100n). That assertion is incorrect: a registration whose grace period has just concluded is fully expired at that exact timestamp, not in some intermediate state. Keepingexpected: truehere (matching the original>=semantics) would correctly document that the boundary is the first instant of full expiry, consistent with howisRegistrationInGracePeriodalready uses a strict upper-bound>to exclude that same instant.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!