[java] Add regression coverage for relative locators on collapsed table borders (#13642) by MohabMohie · Pull Request #17827 · SeleniumHQ/selenium

MohabMohie · 2026-07-27T04:30:56Z

🔗 Related Issues

💥 What does this PR do?

Adds the regression test that #13642 was missing. The underlying bug
(relative locators picking the wrong element in a border-collapse
table) was already fixed by 960ff05 (#14482), released in Selenium
4.28.0, which changed the above/below/left/right proximity
comparisons in the find-elements atom from strict </> to inclusive
<=/>= — but nothing guarded that fix, so a silent regression back to
strict comparisons would go completely undetected today. This PR closes
that coverage gap with one new RelativeLocatorTest scenario, plus two
small follow-ups made in response to review:

Page.withDoctype(): an opt-in flag on the shared test-page builder so
the new test's page can request a standards-mode document (doctype,
<script>/<style> inside <head>) for deterministic parsing,
without changing Page's default output for every other caller.
A PageTest assertion fix: replaced a brittle full-string equality
check with targeted invariant checks (no doctype by default;
<script>/<style> stay after </head> closes; expected content
present) so future harmless whitespace-only refactors of
Page.toString() won't break the test.

No production/runtime code changes are included — the locator fix
already exists on trunk; this PR only adds the missing regression
coverage plus the test-infrastructure changes needed to write it
cleanly.

🔧 Implementation Notes

The new test builds its own inline page via the existing Page
builder (same pattern as ensureNoRepeatedElements), rather than
adding a table to the shared common/src/web/relative_locators.html
fixture. A previous attempt at this ([java][js] Fixed bug #13642 related to relative locators #14336) added such a table to the
shared fixture, which breaks other tests relying on unqualified td
selectors against it (e.g. shouldFindElementsBelowAnother); this PR
avoids that regression by keeping the page test-local.
Page.withDoctype() is opt-in rather than a change to Page's default
shape, because Page is a shared fixture used across the whole test
suite; making it opt-in keeps every existing caller's output
byte-identical while giving the new, layout-sensitive test a
deterministic (non-quirks-mode) document to assert against.
The PageTest fix targets only the one brittle assertion flagged by
review — it still fails if a doctype is reintroduced or <script>/
<style> move back inside <head> (verified against both regression
shapes before applying it), it just no longer pins incidental
whitespace.

🤖 AI assistance

No substantial AI assistance used
AI assisted (complete below)
- Tool(s): Claude Code
- What was generated: the new RelativeLocatorTest scenario and its
  inline test page, the Page.withDoctype() follow-up, the
  PageTest assertion fix, and this PR description. All of it was
  reviewed and verified by me (real bazelisk test runs, including
  deliberate RED/GREEN checks against reverted code, are described
  below); I can explain every change without referring back to the
  tool.
- I reviewed all AI output and can explain the change

💡 Additional Considerations

None outstanding. All review feedback received so far has been addressed
in the follow-up commits described above.

🔄 Types of changes

New feature (non-breaking change which adds functionality and tests!)

Verification

Ran the real JUnit tests via Bazel (bazelisk):

GREEN on current trunk: full RelativeLocatorTest suite (22
tests, including the new one) passes with 0 failures, headless Chrome.
RED against the pre-fix behavior: temporarily reverted the
above/below/left/right comparisons in
javascript/atoms/typescript/find-elements.ts back to strict </>
and reran — the new test fails as expected
(expected: "maria" but was: "mexico"), while reverting back to the
current inclusive comparisons restores the passing state. This
confirms the new test actually exercises the fixed code path and would
catch a regression.
PageTest (2 tests): GREEN with the new invariant-style assertions;
before applying them, confirmed via scratch checks that the same
assertion style throws on both a reintroduced doctype and on
<script>/<style> moved back inside <head> — i.e. it still
catches the regressions the original test guarded against.
Ran scripts/format.sh (no further changes beyond google-java-format
line-wrapping already included in this diff).

Test plan

bazelisk test //java/test/org/openqa/selenium/support/locators:RelativeLocatorTest --//common:headless — 22/22 pass on trunk
Temporarily reverted find-elements.ts to strict comparisons, reran — new test fails with the wrong element, confirming it detects the regression
Reverted the temporary change, reran — back to 22/22 passing
bazelisk test //java/test/org/openqa/selenium/environment:webserver/PageTest --test_output=all — 2/2 pass
scripts/format.sh — no additional formatting changes
CLA bot / CI (will run automatically on this PR)

…le borders (SeleniumHQ#13642) The bug reported in SeleniumHQ#13642 (relative locators picking the wrong element in a border-collapse table) was already fixed by 960ff05 (SeleniumHQ#14482), released in Selenium 4.28.0, which switched the above/below/left/right proximity comparisons from strict </> to inclusive <=/>=. However there was no regression test for it: mirroring all 21 existing Java relative-locator scenarios against a deliberately-reverted strict comparison shows every one of them still passes, meaning a silent regression back to strict comparisons would go completely undetected today. This adds a new test with its own inline page (not the shared common/src/web/relative_locators.html fixture, to avoid breaking other tests that use unqualified `td` selectors against it) modeling a w3schools-style customer table with collapsed borders, and asserts that toRightOf(...).below(...) resolves to the cell that shares a collapsed border with the anchor cells. Verified with a real bazel test run (bazelisk, headless chrome): - GREEN on current trunk (all 22 tests, including the new one, pass) - RED when above/below/left/right in javascript/atoms/typescript/find-elements.ts are temporarily reverted to strict </> (new test fails: expected "maria" but was "mexico") - GREEN again after reverting that temporary change Fixes SeleniumHQ#13642

selenium-ci · 2026-07-27T04:31:22Z

Thank you, @MohabMohie for this code suggestion.

The support packages contain example code that many users find helpful, but they do not necessarily represent
the best practices for using Selenium, and the Selenium team is not currently merging changes to them.

After reviewing the change, unless it is a critical fix or a feature that is needed for Selenium
to work, we will likely close the PR.

We actively encourage people to add the wrapper and helper code that makes sense for them to their own frameworks.
If you have any questions, please contact us

qodo-code-review · 2026-07-27T04:31:48Z

PR Summary by Qodo

Add Java regression test for relative locators with collapsed table borders

🧪 Tests 🕐 10-20 Minutes

AI Description

• Add a regression test for relative locators in border-collapsed HTML tables
• Build an inline table page via the existing Page builder to avoid shared fixture side effects
• Assert toRightOf(...).below(...) selects the adjacent cell sharing a collapsed border

Diagram

graph TD
  A["RelativeLocatorTest"] --> B(["appServer + Page builder"]) --> C["Inline table page"] --> D(["WebDriver (browser)"]) --> E["RelativeLocator.with(...)"] --> F["find-elements atom"]

High-Level Assessment

The following are alternative approaches to this PR:

1. Add the table to the shared relative_locators.html fixture

➕ Reuses an existing, centrally-maintained test page
➕ Avoids inline HTML/CSS in the test source
➖ Higher risk of breaking unrelated tests using broad selectors (e.g., unqualified td queries)
➖ Harder to keep the new scenario isolated and intention-revealing

2. Create a dedicated HTML fixture file for border-collapse tables

➕ Keeps test code smaller and HTML easier to maintain
➕ Fixture can be reused across languages/bindings if needed
➖ Introduces a new shared artifact with potential future coupling
➖ More repo surface area for a single regression scenario

Recommendation: The inline Page-builder approach used in this PR is the best tradeoff: it isolates the border-collapse scenario, avoids destabilizing the shared relative_locators.html fixture, and directly asserts the specific adjacency behavior that would regress if proximity comparisons reverted to strict .

Files changed (1) +37 / -0

Tests (1) +37 / -0

RelativeLocatorTest.javaAdd border-collapse table regression test for relative locators +37/-0

Add border-collapse table regression test for relative locators
• Adds a new JUnit test that serves an inline border-collapsed table via the Page builder. Verifies that combining toRightOf(...) and below(...) selects the intended adjacent td cell (maria) rather than the next-row candidate.
java/test/org/openqa/selenium/support/locators/RelativeLocatorTest.java

qodo-code-review · 2026-07-27T04:34:13Z

Code Review by Qodo

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

Context used

✅ Compliance rules (platform): 18 rules

1. ~~indexOf ordering false-pass~~ ✓ Resolved 🐞 Bug ☼ Reliability ⭐ New

Description

defaultToStringHasNoDoctypeAndMatchesLegacyShape compares indexOf("</head>") to
indexOf("<script")/indexOf("<style") without asserting </head> exists, so if </head> is
removed indexOf becomes -1 and the ordering assertions can pass while the legacy page shape is
broken.

Code

java/test/org/openqa/selenium/environment/webserver/PageTest.java[R34-36]

+    // Legacy shape/ordering contract: <script>/<style> sit after </head> closes, not inside it.
+    assertThat(page.indexOf("</head>")).isLessThan(page.indexOf("<script"));
+    assertThat(page.indexOf("</head>")).isLessThan(page.indexOf("<style"));

Evidence

The test uses page.indexOf(...) comparisons to enforce ordering but never asserts that </head>
(or the other markers) are present; this allows indexOf("</head>") == -1 to satisfy
isLessThan(...) and mask malformed output.

java/test/org/openqa/selenium/environment/webserver/PageTest.java[31-36]

Agent prompt

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

### Issue description
`PageTest.defaultToStringHasNoDoctypeAndMatchesLegacyShape` uses raw `String#indexOf` ordering assertions that can produce false positives when `</head>` is missing (because `indexOf` returns `-1`). This weakens the regression signal of the test.

### Issue Context
This is test-only code, but it can allow regressions in the legacy `Page.toString()` HTML structure to slip through undetected.

### Fix Focus Areas
- java/test/org/openqa/selenium/environment/webserver/PageTest.java[34-36]

### Suggested fix
Replace the `indexOf` comparisons with assertions that both **require presence** and **enforce order**, e.g.:
- `assertThat(page).contains("</head>").contains("<script").contains("<style");`
- `assertThat(page).containsSubsequence("</head>", "<script");`
- `assertThat(page).containsSubsequence("</head>", "<style");`

(Alternatively, store indices in variables and assert each is `>= 0` before comparing.)

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

2. ~~Non-standards test document~~ ✓ Resolved 🐞 Bug ☼ Reliability

Description

The new regression test relies on precise element geometry in a border-collapsed table, but it
builds the page with Page.toString(), which omits a doctype and places <script>/<style> outside
<head>, making document parsing/mode less deterministic for a layout-sensitive assertion. This
increases the risk of a flaky test or reduced cross-browser confidence in the regression signal.

Code

java/test/org/openqa/selenium/support/locators/RelativeLocatorTest.java[R373-404]

+    String url =
+        appServer.create(
+            new Page()
+                .withTitle("Collapsed Borders")
+                .withStyles(
+                    " table {\n"
+                        + "      border-collapse: collapse;\n"
+                        + "      table-layout: fixed;\n"
+                        + "      width: 450px;\n"
+                        + "    }\n"
+                        + "    td, th {\n"
+                        + "      border: 1px solid black;\n"
+                        + "      box-sizing: border-box;\n"
+                        + "      width: 150px;\n"
+                        + "      height: 40px;\n"
+                        + "      padding: 4px;\n"
+                        + "    }")
+                .withBody(
+                    "<table>\n"
+                        + "    <tr><th id=\"company\">Company</th><th"
+                        + " id=\"contact\">Contact</th><th id=\"country\">Country</th></tr>\n"
+                        + "    <tr><td id=\"alfreds\">Alfreds Futterkiste</td><td"
+                        + " id=\"maria\">Maria Anders</td><td id=\"germany\">Germany</td></tr>\n"
+                        + "    <tr><td id=\"centro\">Centro comercial Moctezuma</td><td"
+                        + " id=\"francisco\">Francisco Chang</td><td"
+                        + " id=\"mexico\">Mexico</td></tr>\n"
+                        + "  </table>"));
+    driver.get(url);
+
+    WebElement cell =
+        driver.findElement(with(tagName("td")).toRightOf(By.id("alfreds")).below(By.id("contact")));
+

Evidence
The new test uses appServer.create(new Page()...) for a layout-sensitive border-collapse scenario,
while Page.toString() generates HTML without a doctype and with <script>/<style> after
</head>. Relative locators explicitly rely on client bounding rect geometry; elsewhere in the test
server code, generated pages include an explicit doctype, indicating standards-mode HTML is a
known/used pattern.
java/test/org/openqa/selenium/support/locators/RelativeLocatorTest.java[371-406]
java/test/org/openqa/selenium/environment/webserver/Page.java[61-80]
java/src/org/openqa/selenium/support/locators/RelativeLocator.java[52-55]
java/test/org/openqa/selenium/environment/webserver/GeneratedJsTestHandler.java[43-72]

Agent prompt

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

### Issue description
The new layout/geometry-sensitive regression test constructs its HTML via `Page`, which currently generates non-standards HTML (no `<!DOCTYPE html>` and `</head>` appears before `<script>`/`<style>`). For geometry-based relative-locator assertions, this can make rendering/document-mode less explicit.

### Issue Context
- The test’s purpose is to validate direction/proximity comparisons that depend on client bounding rectangles.
- `Page.toString()` currently emits a minimal HTML skeleton without a doctype and with `script/style` outside the head.

### Fix Focus Areas
- java/test/org/openqa/selenium/support/locators/RelativeLocatorTest.java[371-406]
- java/test/org/openqa/selenium/environment/webserver/Page.java[61-80]

### Suggested fix
Implement one of:
1) Update `Page.toString()` to emit standards HTML (`<!DOCTYPE html>`) and keep `<script>`/`<style>` inside `<head>` (or add a `withDoctype()` / `withStandardsMode()` option defaulting off, then enable it in this new test).
2) If changing `Page` broadly is risky, add a standards-mode option and use it only for this geometry-sensitive test.

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

3. ~~Brittle legacy output assertion~~ ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

PageTest.defaultToStringHasNoDoctypeAndMatchesLegacyShape asserts full-string equality on
Page.toString() including incidental whitespace (e.g., the exact <body  > spacing and a blank
line). This makes future harmless formatting refactors of Page.toString() fail tests even when the
legacy document shape/semantics remain compatible.

Code

java/test/org/openqa/selenium/environment/webserver/PageTest.java[R27-49]

+  void defaultToStringHasNoDoctypeAndMatchesLegacyShape() {
+    String page =
+        new Page().withTitle("Legacy").withScripts("var a = 1;").withStyles("body { }").toString();
+
+    assertThat(page).doesNotContain("<!DOCTYPE");
+    assertThat(page)
+        .isEqualTo(
+            String.join(
+                "\n",
+                "<html>",
+                "<head>",
+                "<title>Legacy</title>",
+                "</head>",
+                "<script type='text/javascript'>",
+                "var a = 1;",
+                "</script>",
+                "<style>",
+                "body { }",
+                "</style>",
+                "<body  >",
+                "",
+                "</body>",
+                "</html>"));

Evidence
The test asserts an exact multi-line HTML string including the precise <body  > spacing and an
empty line for an empty body. That spacing is currently an artifact of how Page.toString() formats
the body tag with two optional attributes, which will produce two spaces when both attributes are
absent.
java/test/org/openqa/selenium/environment/webserver/PageTest.java[27-49]
java/test/org/openqa/selenium/environment/webserver/Page.java[73-79]

Agent prompt

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

## Issue description
`PageTest.defaultToStringHasNoDoctypeAndMatchesLegacyShape` compares the entire serialized HTML string including whitespace/blank lines. This is likely to create unnecessary churn if `Page.toString()` formatting changes while keeping the same legacy *shape*.

## Issue Context
The test appears intended to ensure the default (no-doctype) output remains backward compatible. If exact whitespace is not part of the contract, the assertion can be made more resilient while still verifying key invariants (no doctype; `<script>`/`<style>` are after `</head>`; expected tags exist in order).

## Fix Focus Areas
- java/test/org/openqa/selenium/environment/webserver/PageTest.java[27-49]

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

Previous review results

Review updated until commit ea01e62

Results up to commit 9e6f12a ⚖️ Balanced

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

1. ~~Non-standards test document~~ ✓ Resolved 🐞 Bug ☼ Reliability

Description

The new regression test relies on precise element geometry in a border-collapsed table, but it
builds the page with Page.toString(), which omits a doctype and places <script>/<style> outside
<head>, making document parsing/mode less deterministic for a layout-sensitive assertion. This
increases the risk of a flaky test or reduced cross-browser confidence in the regression signal.

Code

java/test/org/openqa/selenium/support/locators/RelativeLocatorTest.java[R373-404]

+    String url =
+        appServer.create(
+            new Page()
+                .withTitle("Collapsed Borders")
+                .withStyles(
+                    " table {\n"
+                        + "      border-collapse: collapse;\n"
+                        + "      table-layout: fixed;\n"
+                        + "      width: 450px;\n"
+                        + "    }\n"
+                        + "    td, th {\n"
+                        + "      border: 1px solid black;\n"
+                        + "      box-sizing: border-box;\n"
+                        + "      width: 150px;\n"
+                        + "      height: 40px;\n"
+                        + "      padding: 4px;\n"
+                        + "    }")
+                .withBody(
+                    "<table>\n"
+                        + "    <tr><th id=\"company\">Company</th><th"
+                        + " id=\"contact\">Contact</th><th id=\"country\">Country</th></tr>\n"
+                        + "    <tr><td id=\"alfreds\">Alfreds Futterkiste</td><td"
+                        + " id=\"maria\">Maria Anders</td><td id=\"germany\">Germany</td></tr>\n"
+                        + "    <tr><td id=\"centro\">Centro comercial Moctezuma</td><td"
+                        + " id=\"francisco\">Francisco Chang</td><td"
+                        + " id=\"mexico\">Mexico</td></tr>\n"
+                        + "  </table>"));
+    driver.get(url);
+
+    WebElement cell =
+        driver.findElement(with(tagName("td")).toRightOf(By.id("alfreds")).below(By.id("contact")));
+

Evidence
The new test uses appServer.create(new Page()...) for a layout-sensitive border-collapse scenario,
while Page.toString() generates HTML without a doctype and with <script>/<style> after
</head>. Relative locators explicitly rely on client bounding rect geometry; elsewhere in the test
server code, generated pages include an explicit doctype, indicating standards-mode HTML is a
known/used pattern.
java/test/org/openqa/selenium/support/locators/RelativeLocatorTest.java[371-406]
java/test/org/openqa/selenium/environment/webserver/Page.java[61-80]
java/src/org/openqa/selenium/support/locators/RelativeLocator.java[52-55]
java/test/org/openqa/selenium/environment/webserver/GeneratedJsTestHandler.java[43-72]

Agent prompt

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

### Issue description
The new layout/geometry-sensitive regression test constructs its HTML via `Page`, which currently generates non-standards HTML (no `<!DOCTYPE html>` and `</head>` appears before `<script>`/`<style>`). For geometry-based relative-locator assertions, this can make rendering/document-mode less explicit.

### Issue Context
- The test’s purpose is to validate direction/proximity comparisons that depend on client bounding rectangles.
- `Page.toString()` currently emits a minimal HTML skeleton without a doctype and with `script/style` outside the head.

### Fix Focus Areas
- java/test/org/openqa/selenium/support/locators/RelativeLocatorTest.java[371-406]
- java/test/org/openqa/selenium/environment/webserver/Page.java[61-80]

### Suggested fix
Implement one of:
1) Update `Page.toString()` to emit standards HTML (`<!DOCTYPE html>`) and keep `<script>`/`<style>` inside `<head>` (or add a `withDoctype()` / `withStandardsMode()` option defaulting off, then enable it in this new test).
2) If changing `Page` broadly is risky, add a standards-mode option and use it only for this geometry-sensitive test.

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

Results up to commit 356b0de ⚖️ Balanced

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

1. ~~Brittle legacy output assertion~~ ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

PageTest.defaultToStringHasNoDoctypeAndMatchesLegacyShape asserts full-string equality on
Page.toString() including incidental whitespace (e.g., the exact <body  > spacing and a blank
line). This makes future harmless formatting refactors of Page.toString() fail tests even when the
legacy document shape/semantics remain compatible.

Code

java/test/org/openqa/selenium/environment/webserver/PageTest.java[R27-49]

+  void defaultToStringHasNoDoctypeAndMatchesLegacyShape() {
+    String page =
+        new Page().withTitle("Legacy").withScripts("var a = 1;").withStyles("body { }").toString();
+
+    assertThat(page).doesNotContain("<!DOCTYPE");
+    assertThat(page)
+        .isEqualTo(
+            String.join(
+                "\n",
+                "<html>",
+                "<head>",
+                "<title>Legacy</title>",
+                "</head>",
+                "<script type='text/javascript'>",
+                "var a = 1;",
+                "</script>",
+                "<style>",
+                "body { }",
+                "</style>",
+                "<body  >",
+                "",
+                "</body>",
+                "</html>"));

Evidence
The test asserts an exact multi-line HTML string including the precise <body  > spacing and an
empty line for an empty body. That spacing is currently an artifact of how Page.toString() formats
the body tag with two optional attributes, which will produce two spaces when both attributes are
absent.
java/test/org/openqa/selenium/environment/webserver/PageTest.java[27-49]
java/test/org/openqa/selenium/environment/webserver/Page.java[73-79]

Agent prompt

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

## Issue description
`PageTest.defaultToStringHasNoDoctypeAndMatchesLegacyShape` compares the entire serialized HTML string including whitespace/blank lines. This is likely to create unnecessary churn if `Page.toString()` formatting changes while keeping the same legacy *shape*.

## Issue Context
The test appears intended to ensure the default (no-doctype) output remains backward compatible. If exact whitespace is not part of the contract, the assertion can be made more resilient while still verifying key invariants (no doctype; `<script>`/`<style>` are after `</head>`; expected tags exist in order).

## Fix Focus Areas
- java/test/org/openqa/selenium/environment/webserver/PageTest.java[27-49]

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

…tandards mode Addresses the qodo-code-review bot comment on PR SeleniumHQ#17827: the new shouldFindCellThatSharesACollapsedBorderWithTheAnchorCell test built its page with Page.toString(), which omits a doctype and places <script>/<style> outside <head>, making parsing/quirks-mode less deterministic for a layout-sensitive assertion. Add an opt-in Page.withDoctype() flag instead of changing the global Page.toString() default (Page is a shared fixture used across the whole suite). When unset, output is byte-identical to before (see PageTest, asserted against the exact legacy string). When set, it emits "<!DOCTYPE html>" and moves <script>/<style> inside <head>, mirroring the existing standards-mode shape already used by GeneratedJsTestHandler. Only the one new test opts in.

qodo-code-review · 2026-07-27T06:07:30Z

Code review by qodo was updated up to the latest commit 356b0de

diemol · 2026-07-27T07:14:33Z

Can you please use the PR template we have?

…nt checks defaultToStringHasNoDoctypeAndMatchesLegacyShape previously pinned the exact output of Page.toString() via isEqualTo(), including incidental whitespace (the "<body >" double-space, the blank line for an empty body). That makes harmless formatting refactors of toString() fail the test even when the legacy document shape/semantics are unchanged. Replace it with targeted checks that still prove the same contract: no doctype, <script>/<style> come after </head> closes (the legacy ordering, as opposed to the withDoctype() standards-mode shape), and the expected tags/content are present. Verified these assertions still catch the regressions they guard against (reintroduced doctype, script/style moved back inside <head>) before applying them. Addresses qodo-code-review feedback on PR SeleniumHQ#17827.

MohabMohie · 2026-07-27T07:36:31Z

Reply to #17827 (comment):

Fixed in d931828. Replaced the full-string .isEqualTo(...) assertion in defaultToStringHasNoDoctypeAndMatchesLegacyShape with targeted invariant checks:

doesNotContain("<!DOCTYPE") — default output stays in quirks mode.
indexOf("</head>") < indexOf("<script") and < indexOf("<style") — the legacy ordering contract (script/style after head closes, not inside it, as opposed to the withDoctype() standards-mode shape).
.contains(...) checks for the title, script body, style body, and body/html closing tags.

It no longer pins the exact <body > spacing or the blank line for an empty body, so a harmless whitespace-only refactor of Page.toString() won't break it.

Before applying these, I verified they're still meaningfully strict (not vacuous) by running them against two deliberately regressed shapes as a scratch check: a string with <!DOCTYPE html> reintroduced, and one with <script>/<style> moved back inside <head>. Both throw AssertionError under the new assertions, confirming they'd still catch the regressions this test exists to guard against. Real bazelisk test //java/test/org/openqa/selenium/environment:webserver/PageTest is green (2/2) on the actual implementation.

MohabMohie · 2026-07-27T07:36:51Z

@diemol thanks for the nudge — updated the PR description to actually follow .github/PULL_REQUEST_TEMPLATE.md (Related Issues / What does this PR do / Implementation Notes / AI assistance disclosure / Additional Considerations / Types of changes), rather than the free-form summary I had before. Let me know if anything in there needs more detail.

qodo-code-review · 2026-07-27T07:37:34Z

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

defaultToStringHasNoDoctypeAndMatchesLegacyShape compared indexOf("</head>") to indexOf("<script")/indexOf("<style") without asserting </head> exists. If </head> were removed, indexOf() would return -1, which is still "less than" a positive script/style index, so the ordering assertion silently passed while the legacy page shape was actually broken. Replace the indexOf comparisons with AssertJ containsSubsequence, which asserts presence and order together, so a missing </head> now correctly fails the test instead of passing by coincidence. Verified via a scratch test (removed before this commit) that the old assertion style raised no exception for a hand-built </head>-removed string, and that the new containsSubsequence style does throw for the same string.

MohabMohie · 2026-07-27T08:13:33Z

Reply to #17827 (comment):

Fixed in ea01e62: defaultToStringHasNoDoctypeAndMatchesLegacyShape no longer compares raw indexOf("</head>") to indexOf("<script")/indexOf("<style"). That comparison would silently pass if </head> were removed entirely, since indexOf returns -1, which is still "less than" a positive <script/<style index. Replaced both assertions with AssertJ's containsSubsequence("</head>", "<script") / containsSubsequence("</head>", "<style"), which assert presence and order together, so a missing </head> now correctly fails the test. Verified with a scratch test (removed before committing) that the old style raised no exception on a hand-built </head>-removed string, and that the new containsSubsequence style does throw for that same string. bazelisk test //java/test/org/openqa/selenium/environment:webserver/PageTest --test_output=all is green (3/3, including the pre-existing withDoctype... test).

qodo-code-review · 2026-07-27T08:15:11Z

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

selenium-ci added C-java Java Bindings B-support Issue or PR related to support classes labels Jul 27, 2026

qodo-code-review Bot reviewed Jul 27, 2026

View reviewed changes

Comment thread java/test/org/openqa/selenium/support/locators/RelativeLocatorTest.java

qodo-code-review Bot reviewed Jul 27, 2026

View reviewed changes

Comment thread java/test/org/openqa/selenium/environment/webserver/PageTest.java Outdated

MohabMohie added 2 commits July 27, 2026 10:24

Merge branch 'trunk' into fix-13642-regression-coverage

31b6870

qodo-code-review Bot reviewed Jul 27, 2026

View reviewed changes

Comment thread java/test/org/openqa/selenium/environment/webserver/PageTest.java Outdated

titusfortner merged commit f74a320 into SeleniumHQ:trunk Jul 27, 2026
20 checks passed

Uh oh!

Uh oh!

Conversation

MohabMohie commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Related Issues

💥 What does this PR do?

🔧 Implementation Notes

🤖 AI assistance

💡 Additional Considerations

🔄 Types of changes

Verification

Test plan

Uh oh!

selenium-ci commented Jul 27, 2026

Uh oh!

qodo-code-review Bot commented Jul 27, 2026

PR Summary by Qodo

Uh oh!

qodo-code-review Bot commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Previous review results

Uh oh!

Uh oh!

Uh oh!

qodo-code-review Bot commented Jul 27, 2026

Uh oh!

diemol commented Jul 27, 2026

Uh oh!

MohabMohie commented Jul 27, 2026

Uh oh!

MohabMohie commented Jul 27, 2026

Uh oh!

Uh oh!

qodo-code-review Bot commented Jul 27, 2026

Uh oh!

MohabMohie commented Jul 27, 2026

Uh oh!

qodo-code-review Bot commented Jul 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MohabMohie commented Jul 27, 2026 •

edited

Loading

qodo-code-review Bot commented Jul 27, 2026 •

edited

Loading