Skip to content

fix(browser): exclude DNS errors from consecutive failure limit#1156

Open
vringar wants to merge 3 commits intomasterfrom
fix/dns-error-failure-limit-v2
Open

fix(browser): exclude DNS errors from consecutive failure limit#1156
vringar wants to merge 3 commits intomasterfrom
fix/dns-error-failure-limit-v2

Conversation

@vringar
Copy link
Copy Markdown
Contributor

@vringar vringar commented Apr 4, 2026

Summary

  • DNS resolution errors (dnsNotFound / NXDOMAIN) no longer count toward the consecutive failure limit that stops crawling
  • Extract _is_dns_error() as a named, importable function for testability
  • Add integration test with 110 nonexistent domains verifying crawl continues past failure_limit=5
  • Add unit tests that import and verify the real predicate (not a local copy)

Only dnsNotFound is excluded — DNS timeouts/SERVFAIL intentionally still count as they may indicate real network issues.

Supersedes #1139. Incorporates fixes from adversarial review (VDD methodology).

VDD Review History

  • Round 1: Found predicate test testing a copy instead of real code, unused import
  • Round 2: Predicate extracted as real function, tests import it directly. Remaining "major" (browser restart on DNS errors) is pre-existing architecture, not introduced here.
  • Final verdict: Begrudgingly Adequate (converged)

Test plan

  • test_dns_error_does_not_count_against_failure_limit passes
  • test_is_dns_error_predicate tests the real _is_dns_error function
  • pre-commit run --all-files passes

Copilot AI review requested due to automatic review settings April 4, 2026 21:11
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Excludes Firefox dnsNotFound (NXDOMAIN) neterrors from contributing to the consecutive failure limit that stops crawling, and adds tests to ensure crawls continue through large numbers of nonexistent domains.

Changes:

  • Added _is_dns_error() predicate and used it to skip incrementing failure_count on dnsNotFound.
  • Added an integration test that navigates to 110 .invalid domains to verify no failure-limit abort occurs.
  • Added a unit test that imports and validates the real _is_dns_error() predicate.

Reviewed changes

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

File Description
openwpm/browser_manager.py Adds _is_dns_error() and uses it to exclude dnsNotFound from the failure counter.
test/test_webdriver_utils.py Adds integration + unit tests validating the new DNS-exclusion behavior and predicate correctness.
.gitignore Adds ignore rules for Crosslink/Claude-generated local state.

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

Comment thread openwpm/browser_manager.py Outdated

DNS resolution errors are expected when crawling large domain lists
(e.g. Tranco top-100k) and don't indicate a browser or instrumentation
failure. # Only NXDOMAIN; DNS timeouts/SERVFAIL intentionally still count
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

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

The docstring includes a # ... fragment that reads like an inline comment but is actually part of the public docstring text. Please rewrite that line as a normal sentence (e.g., “Only NXDOMAIN is excluded; DNS timeouts/SERVFAIL still count…”) to avoid confusion and keep generated docs clean.

Suggested change
failure. # Only NXDOMAIN; DNS timeouts/SERVFAIL intentionally still count
failure. Only NXDOMAIN is excluded; DNS timeouts and SERVFAIL
intentionally still count.

Copilot uses AI. Check for mistakes.
Comment thread test/test_webdriver_utils.py Outdated
Comment on lines +1 to +3
import pytest

from openwpm.browser_manager import _is_dns_error
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

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

Tests importing a leading-underscore symbol can be confusing because _is_dns_error reads as “private/internal”. If this predicate is intended to be a stable, importable test surface, consider making it a public name (e.g., is_dns_error / is_dns_resolution_error) or relocating it to a dedicated utils module to clarify intended usage.

Copilot uses AI. Check for mistakes.
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 4, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.16%. Comparing base (a088fd3) to head (cc064fd).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1156      +/-   ##
==========================================
- Coverage   62.21%   62.16%   -0.05%     
==========================================
  Files          40       40              
  Lines        3898     3901       +3     
==========================================
  Hits         2425     2425              
- Misses       1473     1476       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Extract the inline is_dns_error predicate from execute_command_sequence
into a module-level _is_dns_error() function so the test imports and
exercises the real code instead of a local copy. Remove unused
CommandExecutionError import from the test file.
@vringar vringar force-pushed the fix/dns-error-failure-limit-v2 branch from e6d86c9 to 72ea044 Compare April 7, 2026 23:37
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