Skip to content

chore(tests): consolidate unit tests subsumed by property tests #422

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
BYK merged 1 commit into from
Mar 13, 2026
Merged

chore(tests): consolidate unit tests subsumed by property tests #422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,31 @@ Use traditional unit tests only when:
- Testing error messages or specific output formatting
- Integration with external systems (E2E tests)

### Avoiding Unit/Property Test Duplication

When a `*.property.test.ts` file exists for a module, **do not add unit tests that re-check the same invariants** with hardcoded examples. Before adding a unit test, check whether the companion property file already generates random inputs for that invariant.

**Unit tests that belong alongside property tests:**
- Edge cases outside the property generator's range (e.g., self-hosted DSNs when the arbitrary only produces SaaS ones)
- Specific output format documentation (exact strings, column layouts, rendered vs plain mode)
- Concurrency/timing behavior that property tests cannot express
- Integration tests exercising multiple functions together (e.g., `writeJsonList` envelope shape)

**Unit tests to avoid when property tests exist:**
- "returns true for valid input" / "returns false for invalid input" — the property test already covers this with random inputs
- Basic round-trip assertions — property tests check `decode(encode(x)) === x` for all `x`
- Hardcoded examples of invariants like idempotency, symmetry, or subset relationships

When adding property tests for a function that already has unit tests, **remove the unit tests that become redundant**. Add a header comment to the unit test file noting which invariants live in the property file:

```typescript
/**
* Note: Core invariants (round-trips, validation, ordering) are tested via
* property-based tests in foo.property.test.ts. These tests focus on edge
* cases and specific output formatting not covered by property generators.
*/
```

```typescript
import { describe, expect, test, mock } from "bun:test";

Expand Down
117 changes: 27 additions & 90 deletions test/lib/bspatch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
/**
* Unit Tests for TRDIFF10 Binary Patch Application
*
* Note: Core invariants (offtin sign/magnitude, parsePatchHeader non-negative
* values) are tested via property-based tests in bspatch.property.test.ts.
* These tests focus on fixture-based patch application, negative-value rejection
* (property generator clears sign bits), and error messages.
*
* Tests the bspatch implementation against real TRDIFF10 patches
* generated by zig-bsdiff v0.1.19 (stored as test fixtures).
*/
Expand All @@ -9,87 +14,11 @@ import { describe, expect, test } from "bun:test";
import { unlinkSync } from "node:fs";
import { tmpdir } from "node:os";
import { join } from "node:path";
import { applyPatch, offtin, parsePatchHeader } from "../../src/lib/bspatch.js";
import { applyPatch, parsePatchHeader } from "../../src/lib/bspatch.js";

const FIXTURES_DIR = join(import.meta.dir, "../fixtures/patches");

describe("offtin", () => {
test("reads zero", () => {
const buf = new Uint8Array(8);
expect(offtin(buf, 0)).toBe(0);
});

test("reads positive value 1", () => {
const buf = new Uint8Array(8);
buf[0] = 1; // LE: least significant byte
expect(offtin(buf, 0)).toBe(1);
});

test("reads positive value 256", () => {
const buf = new Uint8Array(8);
buf[1] = 1; // 256 in LE
expect(offtin(buf, 0)).toBe(256);
});

test("reads negative value -1", () => {
// zig-bsdiff sign-magnitude: magnitude=1, sign bit set in byte 7
const buf = new Uint8Array(8);
buf[0] = 1;
buf[7] = 0x80; // Sign bit
expect(offtin(buf, 0)).toBe(-1);
});

test("reads negative value -256", () => {
const buf = new Uint8Array(8);
buf[1] = 1; // magnitude = 256
buf[7] = 0x80;
expect(offtin(buf, 0)).toBe(-256);
});

test("reads from offset", () => {
const buf = new Uint8Array(16);
buf[8] = 42;
expect(offtin(buf, 8)).toBe(42);
});

test("reads large positive value", () => {
const buf = new Uint8Array(8);
// 0x01_00_00_00 = 16,777,216 in LE: byte 3 = 0x01
buf[3] = 1;
expect(offtin(buf, 0)).toBe(16_777_216);
});

test("reads value spanning both 32-bit halves", () => {
const buf = new Uint8Array(8);
// Low 32 bits: 0
// High 32 bits: 1 → value = 1 * 2^32 = 4,294,967,296
buf[4] = 1;
expect(offtin(buf, 0)).toBe(4_294_967_296);
});
});

describe("parsePatchHeader", () => {
test("rejects too-small buffer", () => {
expect(() => parsePatchHeader(new Uint8Array(31))).toThrow(
"Patch too small"
);
});

test("rejects invalid magic", () => {
const buf = new Uint8Array(32);
buf.set(new TextEncoder().encode("BSDIFF40"));
expect(() => parsePatchHeader(buf)).toThrow("Invalid patch format");
});

test("rejects negative header values", () => {
const buf = new Uint8Array(32);
buf.set(new TextEncoder().encode("TRDIFF10"));
// Set controlLen to -1 (sign bit in byte 15)
buf[8] = 1;
buf[15] = 0x80;
expect(() => parsePatchHeader(buf)).toThrow("negative length");
});

describe("parsePatchHeader: fixtures", () => {
test("parses valid small fixture header", async () => {
const patchData = new Uint8Array(
await Bun.file(join(FIXTURES_DIR, "small.trdiff10")).arrayBuffer()
Expand All @@ -109,6 +38,26 @@ describe("parsePatchHeader", () => {
expect(header.diffLen).toBeGreaterThanOrEqual(0);
expect(header.newSize).toBe(65_536);
});

test("rejects truncated patch (header claims more data than exists)", () => {
const truncated = new Uint8Array(32);
truncated.set(new TextEncoder().encode("TRDIFF10"));
truncated[8] = 100; // controlLen = 100
expect(() => parsePatchHeader(truncated)).toThrow("exceed file size");
});

test("rejects negative header values", () => {
// Property generator clears sign bits (patched[15] %= 128) so never
// exercises the negative-length error path — this test fills that gap
const buf = new Uint8Array(64);
buf.set(new TextEncoder().encode("TRDIFF10"));
// offtin reads sign from bit 7 of byte 7 within each 8-byte field.
// controlLen is at offset 8, so sign bit is byte 15.
// Need non-zero magnitude + sign bit for a negative result.
buf[8] = 1; // non-zero low byte → magnitude > 0
buf[15] = 0x80; // Set sign bit → negative controlLen
expect(() => parsePatchHeader(buf)).toThrow("negative");
});
});

describe("applyPatch", () => {
Expand All @@ -126,14 +75,12 @@ describe("applyPatch", () => {
try {
const sha256 = await applyPatch(oldPath, patchData, destPath);

// Verify output matches expected new file
const expected = await Bun.file(
join(FIXTURES_DIR, "small-new.bin")
).arrayBuffer();
const actual = await Bun.file(destPath).arrayBuffer();
expect(new Uint8Array(actual)).toEqual(new Uint8Array(expected));

// Verify SHA-256 matches
const expectedHash = new Bun.CryptoHasher("sha256")
.update(new Uint8Array(expected))
.digest("hex");
Expand Down Expand Up @@ -185,7 +132,6 @@ describe("applyPatch", () => {

try {
const sha256 = await applyPatch(oldPath, patchData, destPath);
// Known SHA-256 of "Hello, World! This is the new file content. Version 2."
expect(sha256).toMatch(/^[0-9a-f]{64}$/);
expect(sha256.length).toBe(64);
} finally {
Expand All @@ -197,15 +143,6 @@ describe("applyPatch", () => {
}
});

test("rejects truncated patch (header claims more data than exists)", () => {
// Header says controlLen=100 but patch is only 32 bytes
const truncated = new Uint8Array(32);
truncated.set(new TextEncoder().encode("TRDIFF10"));
truncated[8] = 100; // controlLen = 100

expect(() => parsePatchHeader(truncated)).toThrow("exceed file size");
});

test("rejects non-TRDIFF10 magic", async () => {
const oldPath = join(FIXTURES_DIR, "small-old.bin");
const buf = new Uint8Array(64);
Expand Down
96 changes: 8 additions & 88 deletions test/lib/db/auth.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
/**
* Auth Environment Variable Tests
*
* Tests for SENTRY_AUTH_TOKEN and SENTRY_TOKEN env var support.
* Verifies priority, source tracking, and interaction with stored tokens.
* Note: Core invariants (priority, source tracking, refresh skip, isEnvTokenActive)
* are tested via property-based tests in auth.property.test.ts. These tests focus on
* edge cases (whitespace, empty strings), shape assertions, and functions not covered
* by property tests (isAuthenticated, getActiveEnvVarName).
*/

import { afterEach, beforeEach, describe, expect, test } from "bun:test";
import {
type AuthSource,
getActiveEnvVarName,
getAuthConfig,
getAuthToken,
Expand All @@ -20,7 +21,6 @@ import { useTestConfigDir } from "../../helpers.js";

useTestConfigDir("auth-env-");

/** Save and restore env vars around each test */
let savedAuthToken: string | undefined;
let savedSentryToken: string | undefined;

Expand All @@ -32,7 +32,6 @@ beforeEach(() => {
});

afterEach(() => {
// Restore — never leave env vars dangling
if (savedAuthToken !== undefined) {
process.env.SENTRY_AUTH_TOKEN = savedAuthToken;
} else {
Expand All @@ -45,34 +44,7 @@ afterEach(() => {
}
});

describe("env var auth: getAuthToken", () => {
test("returns SENTRY_AUTH_TOKEN when set", () => {
process.env.SENTRY_AUTH_TOKEN = "sntrys_token_abc";
expect(getAuthToken()).toBe("sntrys_token_abc");
});

test("returns SENTRY_TOKEN when set", () => {
process.env.SENTRY_TOKEN = "sntrys_token_xyz";
expect(getAuthToken()).toBe("sntrys_token_xyz");
});

test("SENTRY_AUTH_TOKEN takes priority over SENTRY_TOKEN", () => {
process.env.SENTRY_AUTH_TOKEN = "auth_token";
process.env.SENTRY_TOKEN = "sentry_token";
expect(getAuthToken()).toBe("auth_token");
});

test("env var takes priority over stored token", () => {
setAuthToken("stored_token");
process.env.SENTRY_AUTH_TOKEN = "env_token";
expect(getAuthToken()).toBe("env_token");
});

test("falls back to stored token when no env var", () => {
setAuthToken("stored_token");
expect(getAuthToken()).toBe("stored_token");
});

describe("env var auth: getAuthToken edge cases", () => {
test("ignores empty SENTRY_AUTH_TOKEN", () => {
process.env.SENTRY_AUTH_TOKEN = "";
setAuthToken("stored_token");
Expand All @@ -91,30 +63,7 @@ describe("env var auth: getAuthToken", () => {
});
});

describe("env var auth: getAuthConfig", () => {
test("returns env source for SENTRY_AUTH_TOKEN", () => {
process.env.SENTRY_AUTH_TOKEN = "test_token";
const config = getAuthConfig();
expect(config).toBeDefined();
expect(config?.token).toBe("test_token");
expect(config?.source).toBe("env:SENTRY_AUTH_TOKEN" satisfies AuthSource);
});

test("returns env source for SENTRY_TOKEN", () => {
process.env.SENTRY_TOKEN = "test_token";
const config = getAuthConfig();
expect(config).toBeDefined();
expect(config?.token).toBe("test_token");
expect(config?.source).toBe("env:SENTRY_TOKEN" satisfies AuthSource);
});

test("returns oauth source for stored token", () => {
setAuthToken("stored_token");
const config = getAuthConfig();
expect(config).toBeDefined();
expect(config?.source).toBe("oauth" satisfies AuthSource);
});

describe("env var auth: getAuthConfig shape", () => {
test("env config has no refreshToken or expiry", () => {
process.env.SENTRY_AUTH_TOKEN = "test_token";
const config = getAuthConfig();
Expand All @@ -135,21 +84,7 @@ describe("env var auth: isAuthenticated", () => {
});
});

describe("env var auth: isEnvTokenActive", () => {
test("returns true for SENTRY_AUTH_TOKEN", () => {
process.env.SENTRY_AUTH_TOKEN = "test_token";
expect(isEnvTokenActive()).toBe(true);
});

test("returns true for SENTRY_TOKEN", () => {
process.env.SENTRY_TOKEN = "test_token";
expect(isEnvTokenActive()).toBe(true);
});

test("returns false when no env var set", () => {
expect(isEnvTokenActive()).toBe(false);
});

describe("env var auth: isEnvTokenActive edge case", () => {
test("returns false for empty env var", () => {
process.env.SENTRY_AUTH_TOKEN = "";
expect(isEnvTokenActive()).toBe(false);
Expand Down Expand Up @@ -178,23 +113,8 @@ describe("env var auth: getActiveEnvVarName", () => {
});
});

describe("env var auth: refreshToken", () => {
test("returns env token without refreshing", async () => {
process.env.SENTRY_AUTH_TOKEN = "env_token";
const result = await refreshToken();
expect(result.token).toBe("env_token");
expect(result.refreshed).toBe(false);
});

test("returns env token even with force=true", async () => {
process.env.SENTRY_AUTH_TOKEN = "env_token";
const result = await refreshToken({ force: true });
expect(result.token).toBe("env_token");
expect(result.refreshed).toBe(false);
});

describe("env var auth: refreshToken edge cases", () => {
test("env token skips stored token entirely", async () => {
// Store a token that would require refresh (expired)
setAuthToken("stored_token", -1, "refresh_token");
process.env.SENTRY_AUTH_TOKEN = "env_token";
const result = await refreshToken();
Expand Down
Loading
Loading