review improvements (Jan)

Author: s1gr1d

.agents/skills/write-tests/SKILL.md

@@ -102,6 +102,7 @@ it('handles the request', async () => {
 // Good: asserts on the observable outcome
 it('sets transaction name from route path', () => {
   responseHandler(createMockContext(200));
+
   expect(mockSetTransactionName).toHaveBeenCalledWith('GET /test');
 });
 ```
@@ -111,9 +112,9 @@ it('sets transaction name from route path', () => {
 Default to exact matching. `toMatchObject`, `expect.objectContaining`, and `expect.arrayContaining`
 silently ignore fields that matter. This has caused real bugs to ship in this codebase.
 
-**Use `toEqual` unless you have a specific reason not to.** The same applies to
-`toHaveBeenCalledWith` — spell out every argument rather than wrapping in `objectContaining`.
-This is the single most common place where loose assertions creep in:
+**Use `toEqual` by default.** The same applies to `toHaveBeenCalledWith` — spell out every
+argument rather than wrapping in `objectContaining`. This is the single most common place where
+loose assertions creep in:
 
 ```typescript
 // Bad: silently ignores any missing or extra properties in the call
@@ -129,8 +130,13 @@ expect(startSpan).toHaveBeenCalledWith({
 });
 ```
 
-When you genuinely can't enumerate all fields (e.g., a large framework-generated object), fall
-back to individual `.toBe()` checks on the fields that matter:
+The only valid reasons to use `toMatchObject` or `objectContaining` are: **(1)** the object is
+generated by a framework or third-party library and contains fields you don't control (timestamps,
+random IDs, internal framework state), or **(2)** the object has 10+ fields and the test only
+cares about 2–3 of them (in which case individual `.toBe()` checks on those fields are still
+preferred). If you wrote the object being asserted, you can spell it out — use `toEqual`.
+
+When you do fall back, prefer individual `.toBe()` checks over `objectContaining`:
 
 ```typescript
 expect(event.transaction).toBe('GET /users/:id');
@@ -183,7 +189,7 @@ const url = 'https://api.example.com/users/42?include=profile&format=json';
 
 ### Boundary Value Analysis
 
-If the valid range is 1–100, test 0, 1, 2, 99, 100, 101. Bugs cluster at boundaries — off-by-one
+If the valid range is 1–100, test -2, -1, 0, 1, 2, 99, 100, 101, Number.POSITIVE_INFINITY. Bugs cluster at boundaries — off-by-one
 errors, inclusive/exclusive confusion, type coercion.
 
 ### Test the unhappy path as hard as the happy path
@@ -203,7 +209,8 @@ Each edge case gets its own test with a descriptive name.
 
 - Name test files `*.test.ts`, mirroring the source path: `src/shared/patchRoute.ts` →
   `test/shared/patchRoute.test.ts`.
-- Import from the source path (`../../src/...`), not from the package's published API.
+- Import the module under test from its source path (`../../src/...`). But when importing from a _different_ package
+  (e.g., `@sentry/core` in a `@sentry/node` test), use the package name — that's a real dependency, not the code under test.
 - For browser-environment tests: `/** @vitest-environment jsdom */` at top of file.
 
 ### Mocking
@@ -279,7 +286,7 @@ beforeEach(() => {
 
 ### Grouping
 
-Two levels of `describe` is usually enough. Deeper nesting makes tests harder to find and read.
+1-2 levels of `describe` is usually enough. Deeper nesting makes tests harder to find and read.
 
 ```typescript
 describe('patchRoute', () => {