Review: urlTracker.test.ts

urlTracker-test-review.md

Review: urlTracker.test.ts

Summary

Overall this is a solid test suite that covers the core functionality well. The tests are mostly focused on behavior, have clear names, and are independent. Mutation testing confirmed backward() behavior is properly tested.

Issues & Recommendations

1. Test #10 "should expose readonly navigationHistory" doesn't actually test readonly

it("should expose readonly navigationHistory", () => {
    const tracker = useUrlTracker<string>({ root: "/root" });
    tracker.forward("/folder1");
    tracker.forward("/folder2");

    const history = tracker.navigationHistory.value;
    expect(history).toEqual(["/folder1", "/folder2"]);

    // Verify it's readonly (TypeScript would catch this at compile time)
    expect(tracker.navigationHistory.value).toHaveLength(2);
});

Problem: The comment says "TypeScript would catch this" but doesn't actually test the readonly behavior. The second assertion just re-checks length.

Recommendation: Either:

• Remove this test (TypeScript already enforces readonly at compile time)
• Or actually test that mutations don't affect internal state:

it("should expose readonly navigationHistory", () => {
    const tracker = useUrlTracker<string>({ root: "/root" });
    tracker.forward("/folder1");

    const history = tracker.navigationHistory.value;
    // Attempt mutation shouldn't affect internal state
    (history as string[]).push("/hacked");

    expect(tracker.navigationHistory.value).toEqual(["/folder1"]);
});

2. Test #13 "should update current reactively" is redundant

This test duplicates assertions already covered by:

• Test #2 "should track string URLs through navigation stack"
• Test #12 "should handle multiple forward navigations..."

Recommendation: Delete this test - it adds no value.

3. Test #15 "should maintain correct current value throughout navigation lifecycle" is redundant

Nearly identical to test #2 and #12. The only difference is calling forward("/new-path") at the end after returning to root.

Recommendation: If you want to keep this edge case (forward after backward), make it a focused test:

it("should allow forward navigation after returning to root", () => {
    const tracker = useUrlTracker<string>({ root: "/root" });

    tracker.forward("/level1");
    tracker.backward();
    tracker.forward("/new-path");

    expect(tracker.current.value).toBe("/new-path");
    expect(tracker.navigationHistory.value).toEqual(["/new-path"]);
});

4. Test #11 "should support push/pop aliases" could be minimal

The test duplicates a lot of what's tested in other tests. Since push/pop are just aliases, a minimal test suffices:

it("should support push/pop as aliases for forward/backward", () => {
    const tracker = useUrlTracker<string>({ root: "/root" });

    tracker.push("/folder");
    expect(tracker.current.value).toBe("/folder");

    const result = tracker.pop();
    expect(result).toBe("/root");
});

5. Missing edge case: reset() after backwardWithContext()

No test verifies reset works correctly after partial navigation with backwardWithContext. Could add:

it("should reset correctly after using backwardWithContext", () => {
    const tracker = useUrlTracker<string>({ root: "/root" });
    tracker.forward("/a");
    tracker.forward("/b");
    tracker.backwardWithContext();
    tracker.reset();

    expect(tracker.current.value).toBe("/root");
    expect(tracker.navigationHistory.value).toEqual([]);
});

But honestly this isn't critical since reset() is well-tested already.

---

Proposed Refactoring for Duplication

Several tests repeat the setup pattern. A helper could reduce noise:

function createTrackerWith<T>(root: T, ...items: T[]) {
    const tracker = useUrlTracker<T>({ root });
    items.forEach(item => tracker.forward(item));
    return tracker;
}

// Usage:
it("backward returns previous item", () => {
    const tracker = createTrackerWith("/root", "/a", "/b");
    expect(tracker.backward()).toBe("/a");
});

This is optional - the current tests are readable enough.

---

Tests to Consider Removing

Test	Reason
#10 "should expose readonly navigationHistory"	Doesn't test what it claims; TypeScript handles this
#13 "should update current reactively"	Fully covered by #2 and #12
#15 "should maintain correct current value throughout navigation lifecycle"	Mostly redundant; refocus if kept

---

Unresolved Questions

1. Should we test Vue reactivity specifically (e.g., using watch or watchEffect to verify reactive updates trigger)?
2. Keep test #10 (readonly) or delete since TypeScript enforces it?