Skip to content

[py] wire print_pdf_tests.py into the browser test suites - #17976

Merged
diemol merged 1 commit into
trunkfrom
py-enable-print-pdf-tests
Sep 3, 2026
Merged

diemol merged 1 commit into
trunkfrom
py-enable-print-pdf-tests

Conversation

@diemol

@diemol diemol commented Sep 3, 2026

Copy link
Copy Markdown
Member

🔗 Related Issues

Addresses the second half of #17975 (print_pdf_tests.py unreachable from any Bazel target).

💥 What does this PR do?

py/test/selenium/webdriver/common/print_pdf_tests.py (4 tests) was excluded from every test-<browser>-common / test-<browser>-remote-common glob via an explicit exclude=[...] entry, and was never included in any other target, so it never ran in CI for chrome/edge/firefox/safari.

Root cause: PrintOptions was carved out into its own py_library (:common_print_page_options, py/BUILD.bazel:384) — the same pattern used for alert.py, fedcm/, timeouts.py, etc. — but, unlike those, print_pdf_tests.py was never registered in FEATURE_SUITE_DEFS. It was instead just excluded outright, presumably because the file would otherwise fail to collect (ModuleNotFoundError: No module named 'selenium.webdriver.common.print_page_options') in the common suite, which doesn't depend on that library.

This also meant the webkitgtk/wpewebkit targets — which don't carry the same explicit exclude — silently included the file without the required dependency, so //py:test/selenium/webdriver/common/print_pdf_tests-webkitgtk and -wpewebkit existed in the build graph but would fail on collection if anyone ever ran them (nothing in CI does).

Fix: follow the existing feature-suite pattern instead of excluding outright — add PRINT_TESTS, fold it into FEATURE_TESTS (so it's excluded from the common globs, same as alerts/fedcm/etc.), and register "print": (PRINT_TESTS, ":common_print_page_options") in FEATURE_SUITE_DEFS. This generates test-<browser>-print and test-<browser>-remote-print targets with the correct dependency, wired into the existing test-<browser> / test-<browser>-remote aggregates and the webkitgtk/wpewebkit suites, same as every other feature.

🔧 Implementation Notes

  • Considered adding :common_print_page_options directly to :common's deps instead, but that would pull it into every browser test target regardless of whether it's needed, going against the existing convention (each common_* library exists specifically so unrelated targets aren't invalidated by changes to it — see the comment at py/BUILD.bazel:860-862).
  • Cross-checked other bindings: Java (PrintPageTest.java), .NET (PrintTests.cs), Ruby (print_options_spec.rb) and JS (print_pdf_test.js) all have an equivalent test that runs normally in CI, so Python was the outlier here.
  • Verified locally with bazel test (JDK/output-base overrides omitted below for brevity):
    • //py:test/selenium/webdriver/common/print_pdf_tests-chrome-print — 4/4 passed
    • //py:test/selenium/webdriver/common/print_pdf_tests-firefox-print — 4/4 passed
    • //py:test/selenium/webdriver/common/print_pdf_tests-edge-print — 4/4 passed
    • //py:test/selenium/webdriver/common/print_pdf_tests-safari-print — 4/4 xfail (existing @pytest.mark.xfail_safari markers, expected)
    • //py:test/selenium/webdriver/common/print_pdf_tests-chrome-remote-print and -firefox-remote-print — both passed
  • Did not touch ie_launcher_tests.py (the other file flagged in Python unit tests do not run on pull requests — intended? #17975) — that one traces back to a deliberate PR ([build] do not create targets for IE for browser tests #17548) removing IE test-target generation as IE support is being phased out, so it's a separate, intentional situation rather than an oversight.

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s): Claude Code
    • What was generated: investigated the reachability issue with bazel query/bazel test, diagnosed the missing :common_print_page_options dependency, and wrote the BUILD.bazel fix following the existing FEATURE_SUITE_DEFS pattern.
    • I reviewed all AI output and can explain the change

💡 Additional Considerations

None — this only changes which existing, already-passing tests are wired into the build graph; no test or production code was modified.

🔄 Types of changes

  • Bug fix (backwards compatible)

print_pdf_tests.py imports PrintOptions from its own library
(:common_print_page_options), which was never added as a dependency
of the test-<browser>-common targets, so the file was excluded from
every glob() that feeds them instead. That left it excluded from
Bazel's target graph entirely for chrome/edge/firefox/safari, and
reachable-but-broken (ImportError on collection) for the
webkitgtk/wpewebkit targets, which don't have the same exclude.

Follow the existing feature-suite pattern (fedcm, timeouts, alerts,
etc.) instead: carve print_pdf_tests.py out via PRINT_TESTS, fold it
into FEATURE_TESTS so it's excluded from the common globs, and
register it in FEATURE_SUITE_DEFS so per-browser and per-browser
remote targets are generated with the :common_print_page_options dep.

Verified locally with `bazel test`: 4/4 pass on chrome, firefox and
edge (both local and remote variants); safari correctly xfails via
the existing @pytest.mark.xfail_safari markers.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014kEM1tVUYCgzyhJ5xmseyF
@qodo-code-review

Copy link
Copy Markdown
Contributor

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@selenium-ci selenium-ci added C-py Python Bindings B-build Includes scripting, bazel and CI integrations labels Sep 3, 2026
@diemol
diemol merged commit 05423ea into trunk Sep 3, 2026
44 checks passed
@diemol
diemol deleted the py-enable-print-pdf-tests branch September 3, 2026 07:56
This was referenced Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations C-py Python Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants