test: SimpleCov 100% coverage gate + fix Android device_screen_size bug

[AkashBrowserStack]

What & why

The SDK had minitest specs but no coverage instrumentation or gate. This adds SimpleCov with a 100% line gate and raises coverage to 100% (704/704), verified on Ruby 2.6.

Coverage tooling

The suite runs one spec per process (for file in specs/*.rb; ruby $file), so:

• simplecov added to the Gemfile.
• specs/support/spec_helper.rb starts SimpleCov per process (unique command_name → results merge).
• specs/support/run_spec.rb loads the helper then the spec (a -r flag can't load simplecov before bundler sets up the path).
• specs/support/coverage_check.rb collates the merged results and enforces minimum_coverage line: 100.
• CI runs each spec via run_spec.rb, then coverage_check.rb as the gate.

Source bug fixed (found by the new coverage runs)

Percy::AndroidMetadata#device_screen_size returned symbol-keyed hashes while every caller (and IosMetadata) reads string keys — on Android this silently produced 1×1 tiles (|| 1 fallback) and crashed get_regions_by_location (Integer >= nil). Fixed to return string keys; updated the one spec asserting symbol keys.

Tests

Added tests covering all 52 previously-uncovered lines across cli_wrapper, percy_automate (new spec), app_automate, generic_provider, app_percy, driver_metadata, ios_metadata, metadata, common, and the top-level percy_screenshot error paths. Restored monkey-patched metadata methods in generic_providers teardown to remove pre-existing run-order flakiness. Also made the existing execute_percy_screenshot_end stub Ruby-3.0-kwarg-safe.

Verification

Ruby 2.6 (local): all specs 0 failures/0 errors; collate gate exit 0
Line Coverage: 100.0% (704 / 704)

⚠️ Pre-existing Ruby 3.0 matrix debt (out of scope)

This repo's Test workflow has been red on main for months on the Ruby 3.0 matrix entry, from pre-existing issues unrelated to coverage — e.g. appium_lib API/version drift (uninitialized constant Appium::Core::Base::SearchContext on the 3.0-resolved gem set) and Ruby 3.0 keyword-argument separation. This PR fixes the one 3.0 kwarg issue it touched, but fully greening the 3.0 matrix is a separate dependency-modernization effort (and isn't reproducible without a Ruby 3.0 toolchain). The coverage gate and 100% are delivered for the working Ruby versions; the 3.0 jobs remain pre-existingly red.

🤖 Generated with Claude Code

[AkashBrowserStack]

Claude Code PR Review

PR: #32 • Head: 5d891a3 • Reviewers: stack:code-reviewer (acting per .claude/agents/stack:code-reviewer.md)

Summary

Adds a SimpleCov 100%-line-coverage gate (per-process spec_helper.rb + per-spec run_spec.rb runner + coverage_check.rb collator), wires the gate into CI with fail-fast: false on the Ruby matrix, fixes a real Android bug (device_screen_size now returns string keys 'width'/'height' instead of symbol keys to match all consumers), and adds tests across app_automate, app_percy, cli_wrapper, driver_metadata, generic_providers, ios_metadata, metadata, screenshot, and a new percy_automate spec to reach 100% coverage.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	Only test-fixture hub URLs (hub-cloud.browserstack.com) and localhost CLI stubs; no real secrets.
High	Security	Authentication/authorization checks present	N/A	Test-only changes plus a key-type fix in an SDK; no auth surface.
High	Security	Input validation and sanitization	Pass	app_percy.rb TypeErrors now have explicit tests (test_throws_error_when_string_arg_type_mismatch); no new input paths.
High	Security	No IDOR — resource ownership validated	N/A	No multi-tenant resource access in this gem.
High	Security	No SQL injection (parameterized queries)	N/A	No database/SQL in this gem.
High	Correctness	Logic is correct, handles edge cases	Pass	device_screen_size symbol-to-string fix is correct: every consumer (generic_provider.rb:58-59,180-181, app_automate.rb:60, android_metadata.rb:34) reads string keys, so the old symbol keys silently fell through to || 1. Suite green: 0 failures / 0 errors.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	New tests assert the existing rescue paths (begin/end/screenshot, post_failed_event) explicitly; no new swallowing introduced. Production swallow behavior is unchanged and now characterized.
High	Correctness	No race conditions or concurrency issues	Pass	generic_providers.rb teardown restores monkey-patched session_id/get_system_bars to fix order-dependent flakiness — improves determinism.
Medium	Testing	New code has corresponding tests	Pass	The only production change (android_metadata.rb) is directly tested (test_device_screen_size_when_device_screen_size_is_nil). Harness code (runner/collator) is CI plumbing.
Medium	Testing	Error paths and edge cases tested	Pass	Adds rescue/re-raise, non-200, network-failure, unsupported-version, viewport-top, os_version-error, and ignore_errors true/false paths.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Ran full suite via run_spec.rb: all specs 0 failures / 0 errors; collated minimum_coverage line: 100 passes at 100.0% (704/704), exit 0.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No queries; merge_timeout 3600 is SimpleCov result merging, not runtime.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to an SDK test/CI change.
Medium	Quality	Follows existing codebase patterns	Pass	Minitest + WebMock + Minitest::Mock, consistent with the existing suite; string-keyed hashes now match the rest of the metadata layer.
Medium	Quality	Changes are focused (single concern)	Pass	Bundles coverage gate + a related bug fix surfaced by the gate + a CI fail-fast tweak; all in service of the coverage objective and explained in comments.
Low	Quality	Meaningful names, no dead code	Pass	Clear test/helper names; build_app_driver helper removes duplication; no dead code.
Low	Quality	Comments explain why, not what	Pass	Comments justify intent (fail-fast rationale, pristine-method restore, per-process SimpleCov, line citations) rather than restating code.
Low	Quality	No unnecessary dependencies added	Pass	simplecov added with require: false; justified by the coverage gate. Note: it is unpinned (see Findings, Low).

Findings

• File: Gemfile:42

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: gem 'simplecov', require: false is unpinned, unlike every other gem in the Gemfile (which pin with ~>). The Ruby/Rails convention doc requires pinning all gem versions; an unpinned coverage tool could drift and either change the measured percentage or break the 100% gate on a future release.

• Suggestion: Pin it, e.g. gem 'simplecov', '~> 0.22', require: false, with a short comment noting it backs the CI coverage gate.

• File: specs/app_automate.rb:144-147

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: In test_screenshot_success_sets_metadata_and_debug_url, a @mock_webdriver.expect(:capabilities, get_android_capabilities) is set up between two assertions (after the device-name assert, before the os-version assert). This interleaving of mock setup and assertions is slightly harder to follow than grouping expectations up front; it works and the suite is green, so it is purely stylistic.

• Suggestion: Optionally group the mock expectation with the others before the assertion block for readability. No behavioral change required.

• File: specs/screenshot.rb:660, specs/app_percy.rb:262

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: Several tests use magic repetition counts on mocks (20.times { ... :capabilities }, 3.times { ... }) tied to current internal call counts. These are inherently brittle to future refactors of the production call sequence, though acceptable for Minitest::Mock characterization tests.

• Suggestion: Consider stub over fixed-count expect where the call count is not the thing under test, to reduce coupling. Non-blocking.

---

Verdict: PASS

[pranavz28]

Specs not running on the PR

[AkashBrowserStack]

Claude Code PR Review

PR: #32 • Head: c818768 • Reviewers: stack:code-reviewer (.claude/agents) + 18-row checklist

Summary

Adds a SimpleCov 100% line-coverage gate (per-process spec_helper + run_spec runner + coverage_check collator wired into CI), runs the Test workflow on pull_request with fail-fast: false, pins appium_console ~> 3.0 / appium_lib ~> 12.0 to fix Ruby 3.0 resolution drift, and ships a real Android bug fix changing device_screen_size to return string keys ('width'/'height') instead of symbol keys. The remaining churn is new/expanded specs covering previously-untested rescue/error and fallback branches across app_automate, app_percy, cli_wrapper, driver_metadata, generic_providers, ios_metadata, metadata, percy_automate, and screenshot.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No secrets/credentials introduced; only public BrowserStack hub URLs in test mocks.
High	Security	Authentication/authorization checks present	N/A	Client SDK gem; no auth/authz surface in scope.
High	Security	Input validation and sanitization	N/A	No user-input handling paths touched.
High	Security	No IDOR — resource ownership validated	N/A	No multi-tenant resource access in an SDK.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL / database access.
High	Correctness	Logic is correct, handles edge cases	Pass	device_screen_size symbol→string keys fix is correct: all consumers (generic_provider.rb:58-59,180-181; android_metadata.rb:34) read ['width']/['height'], and iOS sibling already returns string keys. Old symbol keys silently fell back to default 1.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	New tests exercise rescue paths (begin/end/screenshot, post_failed_event, poa non-200); swallow-vs-reraise behavior is asserted, not changed. No production error handling altered.
High	Correctness	No race conditions or concurrency issues	Pass	Per-process SimpleCov merge via unique command_name; no shared mutable state across the parallel-by-Ruby-version matrix (separate runners).
Medium	Testing	New code has corresponding tests	Pass	The one production change (Android key fix) is asserted in specs/android_metadata.rb; new support scripts are CI tooling.
Medium	Testing	Error paths and edge cases tested	Pass	Substantial new coverage of error/rescue, fallback, and edge branches — the explicit goal of the 100% gate.
Medium	Testing	Existing tests still pass (no regressions)	Pass	generic_providers teardown now restores pristine session_id/get_system_bars (class_eval monkey-patches), fixing prior order-dependent flakiness; suite made more robust.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No data-access paths; test-only loops are bounded.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to an SDK gem.
Medium	Quality	Follows existing codebase patterns	Pass	Minitest + WebMock + Minitest::Mock conventions matched; Gemfile versions pinned with an explanatory comment per repo guidelines.
Medium	Quality	Changes are focused (single concern)	Pass	Coherent theme (coverage gate + the bug it surfaced + Ruby-3.0 pin to keep CI green). Mixed but tightly related; acceptable.
Low	Quality	Meaningful names, no dead code	Pass	Clear helper/test names; no dead code. The duplicated metadata_mock build in test_get_tiles_disable_remote_uploads_fullpage (lines 203-207) is unused but harmless.
Low	Quality	Comments explain why, not what	Pass	Comments justify the appium_lib pin, fail-fast, per-process SimpleCov, and pristine-restore rationale — all "why" oriented.
Low	Quality	No unnecessary dependencies added	Pass	simplecov added require: false, dev/CI only, with rationale; pins are tightenings of existing deps, not new runtime deps.

Findings

No Fail rows. Two non-blocking Low observations (not gating):

• File: specs/app_automate.rb:203-207

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: In test_get_tiles_disable_remote_uploads_fullpage_warns_and_continues, a metadata_mock = Minitest::Mock.new is built with four expect calls but never passed anywhere or .verify'd — the test instead stubs @metadata directly. Dead local.

• Suggestion: Remove the unused metadata_mock block to reduce confusion.

• File: specs/app_automate.rb:166-170

• Severity: Low

• Reviewer: stack:code-reviewer

• Issue: test_screenshot_success_sets_metadata_and_debug_url sets @mock_webdriver.expect(:capabilities, ...) between two assertions; the expectation ordering is subtle and relies on os_version consuming exactly one capabilities call. Works, but is brittle to future changes in os_version.

• Suggestion: Optionally add a @mock_webdriver.verify at the end to make the expectation contract explicit.

These are stylistic and do not affect correctness. The production bug fix is correct and well-targeted, the Ruby-3.0 pin is justified and documented, and the coverage tooling is sound.

---

Verdict: PASS