Skip to content

Add coverage for utility edge cases#1017

Open
PeterDaveHello wants to merge 1 commit into
ChatGPTBox-dev:masterfrom
PeterDaveHello:add-test-coverage-quickwins
Open

Add coverage for utility edge cases#1017
PeterDaveHello wants to merge 1 commit into
ChatGPTBox-dev:masterfrom
PeterDaveHello:add-test-coverage-quickwins

Conversation

@PeterDaveHello

@PeterDaveHello PeterDaveHello commented Jul 18, 2026

Copy link
Copy Markdown
Member

Summary

  • Cover bounded XHR reads, truncation, aborts, request failures, and synchronous setup errors.
  • Cover selector fallback, invalid selectors, and empty inputs.
  • Cover known-site, article, normalization, and nested-content extraction paths.

Why

These utilities feed site adapters and page summaries but previously had no focused regression coverage. The tests protect their public behavior without changing runtime code or coupling to third-party readability internals.

Validation

  • npm test (48 test files)
  • npm run test:coverage on Node.js 22 (36.79% lines, 79.72% branches, 73.06% functions)
  • npm run lint
  • npm run build
  • Required Chromium build artifacts verified
  • git diff --check
  • Manual browser smoke not run; it requires an interactive browser-extension session.

Summary by CodeRabbit

  • Tests
    • Added coverage for content extraction and element selection across common page structures.
    • Added tests for selector fallbacks, invalid selectors, and empty inputs.
    • Added coverage for limited network requests, including size limits, truncation, aborts, and error handling.
    • Improved test isolation by restoring browser-like globals and cleaning up simulated DOM resources.

@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

No new commits to review since the last review.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1e6b019c-3901-42c7-8050-0b396c21b345

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds unit tests for DOM selector fallback, core content extraction, and byte-limited XMLHttpRequest behavior, including cleanup of mocked browser globals.

Changes

Utility Test Coverage

Layer / File(s) Summary
Selector fallback behavior
tests/unit/utils/get-possible-element-by-query-selector.test.mjs
Tests first-match selection, invalid-selector fall-through, and undefined results for unusable selector lists.
Core content extraction
tests/unit/utils/get-core-content-text.test.mjs
Tests browser-global setup, preferred selectors, text normalization, body fallback, largest-child selection, and nested content selection.
Limited XHR handling
tests/unit/utils/limited-fetch.test.mjs
Tests progress limits, truncation, abort behavior, request failures, and synchronous XHR errors.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested labels: Review effort 3/5

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding focused edge-case coverage for utility code.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Add unit test coverage for utility edge cases

🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Add regression tests for bounded XHR reads, truncation, aborts, and XHR failures.
• Add tests for selector fallback behavior, invalid selectors, and empty inputs.
• Add tests for core-content extraction paths (known sites, article, readability, body fallback).
Diagram

graph TD
  T["Unit tests"] --> GCC["getCoreContentText"] --> DOM["DOM globals"]
  GCC --> QS["getPossibleElement..."] --> DOM
  GCC --> RD["Readability"]
  T --> QS
  T --> LF["limitedFetch"] --> XHR["XMLHttpRequest"]

  subgraph Legend
    direction LR
    _t["Test"] ~~~ _u["Utility module"] ~~~ _ext["External/Browser API"]
  end
Loading
High-Level Assessment

The PR’s approach—focused unit tests that stub browser globals (JSDOM for DOM, a minimal fake XHR for limitedFetch)—is the most direct way to lock in public behavior without changing runtime code or introducing heavier mocking dependencies. Considered alternatives (shared test harness helpers or third-party mocking libraries) would add maintenance/dependency overhead without materially improving coverage for these specific edge cases.

Files changed (3) +306 / -0

Tests (3) +306 / -0
get-core-content-text.test.mjsAdd core-content extraction fallback/normalization tests +79/-0

Add core-content extraction fallback/normalization tests

• Introduces JSDOM-backed tests that validate known-site selector preference, article extraction with normalization, and body-based fallbacks (small body vs. largest child). Restores global DOM-related properties after each test to avoid cross-test pollution.

tests/unit/utils/get-core-content-text.test.mjs

get-possible-element-by-query-selector.test.mjsAdd tests for selector iteration and invalid selector handling +67/-0

Add tests for selector iteration and invalid selector handling

• Adds tests ensuring the utility returns the first matching selector, skips missing selectors, and ignores invalid selectors that throw. Also verifies undefined is returned for null/empty inputs and that only truthy selectors are attempted.

tests/unit/utils/get-possible-element-by-query-selector.test.mjs

limited-fetch.test.mjsAdd bounded XHR download tests (truncate/abort/errors) +160/-0

Add bounded XHR download tests (truncate/abort/errors)

• Adds a fake XMLHttpRequest implementation to drive onprogress/onload/onerror behavior and verify truncation/abort semantics at the byte limit. Covers request failure status rejection and synchronous setup errors (constructor/open/send throwing), restoring the global XHR after each test.

tests/unit/utils/limited-fetch.test.mjs

@kilo-code-bot

kilo-code-bot Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (3 files)
  • tests/unit/utils/get-core-content-text.test.mjs
  • tests/unit/utils/get-possible-element-by-query-selector.test.mjs
  • tests/unit/utils/limited-fetch.test.mjs
Reconciled Prior Findings (resolved)
  • get-core-content-text.test.mjs:73 & :89 (Copilot) – Readability short-circuit brittleness: resolved, the test now stubs isProbablyReaderable to () => false at module load (lines 9-11).
  • get-core-content-text.test.mjs:89 (gemini) – missing second-largest-element fallback test: resolved by the "uses a major nested content element" test.
  • limited-fetch.test.mjs:61 (qodo) – unhandled rejection from fulfillment-only .then(): resolved, responsePromise.then(onFulfilled, onRejected) now supplies a rejection handler (lines 55-66).
Previous Review Summaries (2 snapshots, latest commit 2f4f413)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 2f4f413)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (3 files)
  • tests/unit/utils/get-core-content-text.test.mjs
  • tests/unit/utils/get-possible-element-by-query-selector.test.mjs
  • tests/unit/utils/limited-fetch.test.mjs

Previous review (commit 80529d5)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (3 files)
  • tests/unit/utils/get-core-content-text.test.mjs
  • tests/unit/utils/get-possible-element-by-query-selector.test.mjs
  • tests/unit/utils/limited-fetch.test.mjs

Reviewed by hy3:free · Input: 59.9K · Output: 3.3K · Cached: 216.7K

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces new unit test suites for the utility functions getCoreContentText, getPossibleElementByQuerySelector, and limitedFetch. The feedback suggests adding an additional test case to getCoreContentText to cover the fallback scenario where the second largest element's area is greater than 50% of the largest element's area, ensuring complete test coverage.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread tests/unit/utils/get-core-content-text.test.mjs
@qodo-code-review

qodo-code-review Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Informational

1. Unhandled rejection in test ✓ Resolved 🐞 Bug ☼ Reliability
Description
The first limitedFetch test attaches a fulfillment-only responsePromise.then(...) and discards the
returned promise; if limitedFetch() rejects, that derived promise will reject without a handler
and can surface as an unhandled rejection during an already-failing test run. This can add noisy
diagnostics or an additional runner-level failure that obscures the real assertion failure.
Code

tests/unit/utils/limited-fetch.test.mjs[R55-61]

+test('limitedFetch keeps downloading while progress remains below the byte limit', async () => {
+  const requests = installFakeXMLHttpRequest()
+  const responsePromise = limitedFetch('https://example.com/data', 5)
+  let settled = false
+  responsePromise.then(() => {
+    settled = true
+  })
Evidence
The PR introduces a fulfillment-only .then() whose returned promise is not awaited or given a
rejection handler, creating an unhandled-rejection path if limitedFetch() rejects.

tests/unit/utils/limited-fetch.test.mjs[55-61]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
In `limited-fetch.test.mjs`, a fulfillment-only `.then()` is attached to `responsePromise` and the returned promise is ignored. If `responsePromise` rejects, the derived promise rejects without a handler, which can be reported as an unhandled rejection and add noise to failing test output.

### Issue Context
The test uses this `.then()` only to update the `settled` flag; it does not need to propagate rejection.

### Fix Focus Areas
- tests/unit/utils/limited-fetch.test.mjs[55-61]

### Suggested fix
Replace:
```js
responsePromise.then(() => {
 settled = true
})
```
with a handled version, e.g.:
```js
void responsePromise.then(
 () => {
   settled = true
 },
 () => {
   settled = true
 },
)
```
(Or capture/await the returned promise if you prefer explicit handling.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Previous review results

Review updated until commit f6bf53d

Results up to commit 2f4f413


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Informational
1. Unhandled rejection in test ✓ Resolved 🐞 Bug ☼ Reliability
Description
The first limitedFetch test attaches a fulfillment-only responsePromise.then(...) and discards the
returned promise; if limitedFetch() rejects, that derived promise will reject without a handler
and can surface as an unhandled rejection during an already-failing test run. This can add noisy
diagnostics or an additional runner-level failure that obscures the real assertion failure.
Code

tests/unit/utils/limited-fetch.test.mjs[R55-61]

+test('limitedFetch keeps downloading while progress remains below the byte limit', async () => {
+  const requests = installFakeXMLHttpRequest()
+  const responsePromise = limitedFetch('https://example.com/data', 5)
+  let settled = false
+  responsePromise.then(() => {
+    settled = true
+  })
Evidence
The PR introduces a fulfillment-only .then() whose returned promise is not awaited or given a
rejection handler, creating an unhandled-rejection path if limitedFetch() rejects.

tests/unit/utils/limited-fetch.test.mjs[55-61]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
In `limited-fetch.test.mjs`, a fulfillment-only `.then()` is attached to `responsePromise` and the returned promise is ignored. If `responsePromise` rejects, the derived promise rejects without a handler, which can be reported as an unhandled rejection and add noise to failing test output.

### Issue Context
The test uses this `.then()` only to update the `settled` flag; it does not need to propagate rejection.

### Fix Focus Areas
- tests/unit/utils/limited-fetch.test.mjs[55-61]

### Suggested fix
Replace:
```js
responsePromise.then(() => {
 settled = true
})
```
with a handled version, e.g.:
```js
void responsePromise.then(
 () => {
   settled = true
 },
 () => {
   settled = true
 },
)
```
(Or capture/await the returned promise if you prefer explicit handling.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Qodo Logo

@PeterDaveHello
PeterDaveHello force-pushed the add-test-coverage-quickwins branch from 80529d5 to 2f4f413 Compare July 18, 2026 22:24
@PeterDaveHello

Copy link
Copy Markdown
Member Author

/agentic_review

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds focused unit-test coverage for several utility helpers that are used by site adapters and page-summary extraction, aiming to lock in edge-case behavior without changing runtime implementation.

Changes:

  • Added XHR-based bounded fetch tests covering progress-limited truncation, abort behavior, failures, and synchronous setup errors.
  • Added selector fallback tests covering invalid selectors, missing selectors, and empty inputs.
  • Added core-content extraction tests covering known-site selectors, article extraction/normalization, and geometric body fallbacks.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
tests/unit/utils/limited-fetch.test.mjs New regression tests for limitedFetch() around truncation limits and XHR error/exception paths.
tests/unit/utils/get-possible-element-by-query-selector.test.mjs New tests for selector iteration, invalid selector handling, and empty input behavior.
tests/unit/utils/get-core-content-text.test.mjs New tests for known-site/article extraction and body-based fallback selection logic.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/unit/utils/get-core-content-text.test.mjs
Comment thread tests/unit/utils/get-core-content-text.test.mjs
Comment thread tests/unit/utils/limited-fetch.test.mjs Outdated
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 2f4f413

Add focused regression coverage for bounded XHR reads, selector
fallback, and core page-content extraction. These tests protect
user-visible adapter behavior without changing runtime code.
@PeterDaveHello
PeterDaveHello force-pushed the add-test-coverage-quickwins branch from 2f4f413 to f6bf53d Compare July 19, 2026 10:34
@PeterDaveHello

Copy link
Copy Markdown
Member Author

/agentic_review

@PeterDaveHello

Copy link
Copy Markdown
Member Author

@coderabbitai review

@PeterDaveHello
PeterDaveHello requested a review from Copilot July 19, 2026 10:34
@coderabbitai

coderabbitai Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit f6bf53d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants