test: coverlet coverage gate + raise non-driver coverage 28% → 54%

[AkashBrowserStack]

What & why

coverlet was present but unwired (no gate). This enables it (adds coverlet.msbuild) with a 53% line gate in npm test, and adds a mock/HTTP-based test class covering the non-driver logic.

Honest partial coverage: the live-Firefox/WebDriver paths can't be unit-tested without a browser (they run on the CI's full suite). 53% is the deterministic floor for the unit-testable logic, not a faked number.

Coverage: 27.66% → 54.16% line (non-driver, local); higher on the browser CI

New PercyLogicTest (56 tests, xunit + RichardSzalay.MockHttp + the internal setCliConfig seam) covers: GetOrigin, IsUnsupportedIframeSrc, JSON helpers, ResolveReadinessConfig merge, min-height/responsive-target resolution, isResponsiveSnapshotCapture branches, PayloadParser, CreateRegion variants, Enabled healthcheck branches + caching, GetPercyDOM, GetResponsiveWidths, Log.

Gate

• npm test now runs dotnet test /p:CollectCoverage=true /p:ThresholdType=line /p:Threshold=53 (CI runs the full suite under percy exec with Firefox, so coverage there is higher; 53% is the guaranteed floor).
• Uncovered = live-WebDriver only: Snapshot/getSerializedDom/ProcessFrame/WaitForReady/CaptureResponsiveDom/ChangeWindowDimension and PercySeleniumDriver's reflection-on-real-driver paths — exercised by the existing UnitTests class on the browser CI.

Verification

dotnet test (non-driver subset) → Passed! 68, Failed: 0; Total Line 54.16% (≥53 gate, exit 0)

(Full browser suite + gate verify on this PR's CI — Test runs on pull_request.)

🤖 Generated with Claude Code

[AkashBrowserStack]

Claude Code PR Review

PR: #451 • Head: 5a680a0 • Reviewers: stack:code-reviewer

Summary

Test/build-config-only PR: wires up a coverlet line-coverage gate (npm test now runs dotnet test /p:CollectCoverage=true /p:CoverletOutputFormat=cobertura /p:ThresholdType=line /p:Threshold=53), adds the coverlet.msbuild 6.0.2 package + lockfile entry, gitignores coverage artifacts, and adds a new non-driver xunit test class (PercyLogicTest) that lifts non-driver logic coverage from ~28% to ~54%. No production code is changed.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No secrets; mock URLs (localhost:5338) and a dummy user:pass@host.io only as a GetOrigin test input.
High	Security	Authentication/authorization checks present	N/A	Test/build config only; no auth surface.
High	Security	Input validation and sanitization	N/A	No user-input handling introduced.
High	Security	No IDOR — resource ownership validated	N/A	No resource access paths.
High	Security	No SQL injection (parameterized queries)	N/A	No DB access.
High	Correctness	Logic is correct, handles edge cases	Pass	Test expectations match source: GetOrigin userinfo stripping, IsUnsupportedIframeSrc js/data/vbscript schemes, Enabled version/caching branches, ResolveReadinessConfig merge precedence. Threshold semantics correct (/p:Threshold=53 fails build below 53).
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Tests assert the production swallow-and-log paths (Log_SwallowsHttpFailureButStillPrints, Enabled_FalseAndDisablesWhenHealthcheckThrows); no new catch blocks added in product code.
High	Correctness	No race conditions or concurrency issues	Pass	New tests mutate global statics (Percy.Enabled, _http, _dom, _enabled), but xunit.runner.json sets parallelizeAssembly:false + parallelizeTestCollections:false, and Dispose() restores Enabled/HTTP client/session type and resets caches, preventing cross-test leakage. See Findings (Low).
Medium	Testing	New code has corresponding tests	Pass	This PR is the tests; 56 new [Fact]/[Theory] cases over the non-driver surface.
Medium	Testing	Error paths and edge cases tested	Pass	Null/empty/unparsable inputs, malformed-config catch branches, missing version header, HTTP 500, missing widths key all covered.
Medium	Testing	Existing tests still pass (no regressions)	Pass	No product code touched; InternalsVisibleTo("Percy.Test") already present so internal seams are reachable; new class deliberately omits IClassFixture<TestsFixture> to avoid spinning Firefox.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	Test code; HTTP is mocked.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable.
Medium	Quality	Follows existing codebase patterns	Pass	Reuses the existing RichardSzalay.MockHttp + reflection-on-private-statics pattern already used in Percy.Test.cs/PercyDriver.Test.cs; same namespace.
Medium	Quality	Changes are focused (single concern)	Pass	Coverage gate + the tests that satisfy it; tightly scoped.
Low	Quality	Meaningful names, no dead code	Pass	Clear test names; reflection helpers (InvokePrivate, InvokeLog) are well-named and used.
Low	Quality	Comments explain why, not what	Pass	Header comment justifies the no-fixture design and reflection seams; per-test comments explain non-obvious catch/cache branches.
Low	Quality	No unnecessary dependencies added	Pass	Only coverlet.msbuild 6.0.2 (pinned, lockfile updated). Complementary to the pre-existing coverlet.collector; drives the /p:CollectCoverage MSBuild gate.

Findings

No Critical or High findings.

• File: Percy.Test/PercyLogic.Test.cs:65

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: Test isolation relies on two implicit, separately-located guarantees: the [Collection("PercyLogicSerial")] attribute AND the assembly-wide parallelizeTestCollections:false in xunit.runner.json. The collection name is never bound to a CollectionDefinition, so the attribute alone is inert; isolation is actually provided by the runner config. If someone later re-enables parallelization (or removes the runner.json), these global-static-mutating tests could interleave with the live-driver UnitTests and flake. Dispose() restoring Percy.Enabled/HTTP client/caches mitigates most leakage.

• Suggestion: Either drop the inert [Collection] attribute and note that serialization comes from xunit.runner.json, or add a real [CollectionDefinition("PercyLogicSerial")] applied to both this class and UnitTests so isolation is explicit regardless of the runner.json setting.

• File: Percy.Test/PercyLogic.Test.cs:451

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: GetHttpClient_LazilyCreatesDefaultWhenUnset reaches into the private static _http field by reflection and nulls it to force the lazy-creation branch, coupling the test to the field name. A future rename of _http would break this test with an opaque NRE on httpField rather than a clear failure. Acceptable given the repo's reflection-heavy test style, but brittle.

• Suggestion: Consider an internal test seam (e.g. internal static void resetHttpClient()) so the lazy path can be exercised without binding to the private field name, consistent with the other internal setters already exposed for tests.

---

Verdict: PASS

[pranavz28]

+--------+--------+--------+--------+
| Module | Line | Branch | Method |
+--------+--------+--------+--------+
| Percy | 64.81% | 70.54% | 81.48% |
+--------+--------+--------+--------+

+---------+--------+--------+--------+
| | Line | Branch | Method |
+---------+--------+--------+--------+
| Total | 64.81% | 70.54% | 81.48% |
+---------+--------+--------+--------+
| Average | 64.81% | 70.54% | 81.48% |
+---------+--------+--------+--------+

[pranavz28]

Threshold should be 100%

[AkashBrowserStack]

Claude Code PR Review

PR: #451 • Head: d548340 • Reviewers: stack:code-reviewer

Summary

Adds a coverlet line-coverage gate (/p:ThresholdType=line /p:Threshold=91 wired into the npm test script) and a large mock-based test suite that raises non-driver line coverage to ~96%. The suite introduces an in-process FakeWebDriver that overrides Selenium's single ExecuteAsync chokepoint to drive the real, sealed WebDriver/PercySeleniumDriver/PercyDriver code paths without launching a browser, plus @percy/cli HTTP mocking via RichardSzalay.MockHttp. Test-only change: no production (Percy.cs / SDK) source is modified.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	Only the public BrowserStack hub URL appears; no tokens, keys, or passwords.
High	Security	Authentication/authorization checks present	N/A	Test-only diff; no auth surface introduced.
High	Security	Input validation and sanitization	N/A	No new user-input boundaries; tests feed canned fixtures.
High	Security	No IDOR — resource ownership validated	N/A	No resource access paths.
High	Security	No SQL injection (parameterized queries)	N/A	No database access.
High	Correctness	Logic is correct, handles edge cases	Pass	Fakes faithfully model Selenium's W3C protocol (element-6066 keys, Response/SessionId); tests assert both happy and error/edge branches (CORS vs same-origin iframes, malformed src, empty percy-element-id, CDP-throws fallback, timeout restore failure).
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Tests intentionally drive swallow-and-continue branches and assert via Record.Exception(...)==null plus side-effect counters; matches the SDK's best-effort design. No production exception handling changed.
High	Correctness	No race conditions or concurrency issues	Pass	Both test classes share [Collection("PercyLogicSerial")] to serialize mutation of Percy static state (_http, _enabled, sessionType, cliConfig); per-test ctor/Dispose snapshot and restore Enabled, reset caches, and reset the HTTP client, preventing cross-test leakage.
Medium	Testing	New code has corresponding tests	Pass	The change is tests; coverage raised to ~96% line, above the 91 floor.
Medium	Testing	Error paths and edge cases tested	Pass	Strong negative-path coverage: healthcheck success=false / missing version header / unsupported version / transport failure; iframe skip variants; CDP throw fallback; readiness timeout read/restore failures; Log HTTP failure still prints.
Medium	Testing	Existing tests still pass (no regressions)	Pass	New files only; PercyLogicSerial collection prevents interference with the existing live-driver suite, which is excluded via FullyQualifiedName!~UnitTests. CI gate enforced on the full run.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No data-fetch loops; in-memory fakes.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to a test suite.
Medium	Quality	Follows existing codebase patterns	Pass	xUnit + MockHttp consistent with the repo's existing test project; coverlet wired through the standard .csproj PackageReference + lockfile, matching existing tooling conventions.
Medium	Quality	Changes are focused (single concern)	Pass	Scope is exactly "coverage gate + tests"; the .gitignore additions (coverage artifacts, TestResults/) and the package.json test-script change directly support that concern.
Low	Quality	Meaningful names, no dead code	Pass	Test/helper names are descriptive and self-documenting. One minor ordering nuance noted in Findings (a defensively-ordered handler branch), not dead code.
Low	Quality	Comments explain why, not what	Pass	Comments are unusually high-value — they explain why each branch is exercised (which production path each fake response drives), the right altitude for reflection-heavy seams.
Low	Quality	No unnecessary dependencies added	Pass	Single new dev dependency coverlet.msbuild 6.0.2, pinned with content hash in packages.lock.json, scoped PrivateAssets=all; standard .NET coverage tool, appropriate and minimal.

Findings

No Critical, High, or Medium findings.

Low / informational (non-blocking):

• File: Percy.Test/PercyDriverFlow.Test.cs:965

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: In Snapshot_StripsReadinessAndAttachesDiagnostics, the if (cmd == DriverCommand.ExecuteAsyncScript) check is nested inside the outer if (cmd == ExecuteScript || cmd == ExecuteAsyncScript) block and sits after the script-content branches. It is reachable (an async readiness call carries none of the serialize/URL/PercyDOM markers, so it falls through to this line), but the double cmd == nesting reads as redundant and could mislead a future maintainer into thinking it is dead.

• Suggestion: Optional cleanup — hoist the ExecuteAsyncScript diagnostics return to a sibling branch at the top of the handler (alongside the other top-level cmd == checks) rather than nesting it. Behavior is correct as-is; readability only.

• File: package.json:4

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: The coverage floor (/p:Threshold=91) is embedded inline in the npm test script, coupling the gate value to that one invocation path; a CI runner calling dotnet test directly would bypass the threshold.

• Suggestion: Optional — consider moving CollectCoverage/ThresholdType/Threshold into the test .csproj (or Directory.Build.props) so the gate travels with the project regardless of entrypoint. Not required for this PR.

---

Verdict: PASS — test-only change with no production code touched; faithful Selenium fakes, strong happy/error/edge-path coverage to ~96% (above the 91 floor), serialized static-state handling, and a single well-scoped dev dependency. No High row failed and no Critical/High finding.