[May-Mei6]

Internal group review before opening the PR to seleniumboot/selenium-boot. Please approve or leave comments. This PR should not be merged into master, so that master stays in sync with upstream until the changes are accepted there.

The draft PR description is below:

Title

docs(recipe): read and assert on table rows with Locator.rows()

What & why

Adds a recipe for reading and asserting on HTML tables with Locator.rows(), which was added in 54 but had no recipe.

It covers reading all rows, finding a row by column value, asserting on a cell, and the behaviour of short rows and <th>-only group rows. It also adds a Recipes sidebar entry and a 138-character SEO description.

Type of change

• Bug fix
• New feature (opt-in, aligned with the framework philosophy)
• Documentation
• Internal refactor / test coverage

Checklist

• All existing tests pass (mvn clean verify) (mvn clean verify -Dgpg.skip; GPG signing skipped locally)
• New behaviour is covered by unit tests — N/A, docs only
• No new external dependencies introduced without prior discussion
• Code follows existing conventions (no new frameworks, minimal abstraction)
• No breaking changes to @SeleniumBootApi public APIs (or a major version bump is justified below)
• Docs updated if user-facing behaviour changed (docs-site/) — N/A, no behaviour change; this PR is itself a docs addition

Notes for reviewers

Reading rows, finding a row, and asserting on a cell were verified in a local TestNG project extending BaseTest using Chrome:

• The AccountsTableTest class shown on the page was run as-is, apart from open("/accounts") → open("/table.html") and one print line, against a local copy of the table in the recipe, and passed. Changing the cell to $90.00 made the assertion fail with expected [$100.00] but found [$90.00].
• The same read / find / assert steps passed against the first table on https://the-internet.herokuapp.com/tables (full six-column table).

Verify short rows and <th>-only group rows: verified in a separate local TestNG project extending BaseTest, using real Chrome and local HTML tables based on the recipe. All 8 tests passed together:

• Two standalone tests verified short rows and <th>-only group rows respectively.
• Six tests in the combined class verified normal reading, lookup and cell assertion; missing versus explicitly empty middle cells; extra cells beyond the header count; selection of the first row containing <th> as the header when there is no <thead>; short rows and group headings together; and rejection of an incorrect expected cell value.

Build the framework and run existing tests: mvn clean verify -Dgpg.skip=true passed 710 tests on the base used for this verification. We will rerun it after syncing with the latest upstream master and update the count before opening the upstream PR.

Test against the official consumer project: the official consumer project was tested against the local framework build; 111 tests passed and 2 were intentionally skipped.

Build and preview the documentation site: the production build passed. The site was run locally to check page rendering and confirm that the Recipes sidebar entry opened the correct page.

Structure: the page follows the existing recipes, using handle-iframes.md as the template: the main example is a titled, complete test class, and the page ends with a Deeper reference link.

Co-authored with @May-Mei6.

[TONGdamy]

I reviewed both changed files against the acceptance criteria for upstream issue seleniumboot#123.

The recipe covers reading rows, finding a row by column value, asserting a cell, and handling short rows and <th>-only group rows. The edge-case explanations are consistent with the current Locator.rows() implementation. The sidebar entry matches the new page, and the SEO description is 138 characters.

I found no blocking issues in this documentation review. I have read the validation summary in the PR description, but have not independently rerun the tests or documentation build.

Optional suggestion: link the local browser test source and relevant output so the verification is easier to reproduce.

Approved for the documentation changes. As requested, this internal group-review PR should remain unmerged.

[May-Mei6]

Thanks for the review. The snippets in the recipe are the same code we ran, so we’ll keep the local test project out of the PR for now. We can share it if the maintainer asks.

[crystalmountain666]

The recipe meets the acceptance criteria in seleniumboot#123. It covers all five topics, the sidebar entry opens the right page, the SEO description is 138 characters, and the docs site was built and run locally. I also checked each behaviour described on the page against the Javadoc on Locator.rows(), and they match.

Two things about the Notes for reviewers.

The maintainer asked for the snippets to be verified against a real table, not just the Javadoc. Notes 1 and 2 do this for the core snippets and both edge cases. But the page also states three behaviours in prose: a missing middle shifts later values, extra cells are dropped, and without a the first row becomes the header. Were these among the "other behaviours" you tested? The test count also varies: commit 387db2f says 6 tests passed, while bef2230 and the PR description say 8. It would help to list what the 8 tests cover.

The 710-test count comes from the old base. Please rerun mvn clean verify after rebasing onto the current master, which is 9 commits ahead, and update the number. The rebase also removes the two internal merge commits from the history.

The maintainer also asked us to use an existing recipe as the template for structure. The Notes don't say which page you followed, and this one differs from the others in three ways. Every existing recipe gives its main code block a file title and shows the call inside a class. Four of them end with a Deeper reference: link. They are also between 20 and 53 lines long, compared with 129 lines here, mostly because the instruction to replace only is repeated three times. Since the maintainer offered a structure check on a draft PR, these could be fixed now or raised with him.

I'd approve once the test coverage question is answered. The template points can be discussed.

[May-Mei6]

The mvn clean verify test count in the description is provisional. We’ll sync with the latest upstream master, rerun the build and update the number right before opening the PR to seleniumboot/selenium-boot, so we don’t have to keep chasing upstream changes in the meantime.

[Lisr-iiil]

Sorry for the accidental closure! I clicked “Close with comment” without realising it would close the whole PR. Lesson learned: the button meant exactly what it said 🙂 I’ve reopened it. Thanks for your patience and for continuing the review!

[Lisr-iiil]

@crystalmountain666 Thanks for the detailed review. To clarify the three points:

1. The additional behaviours were tested. Although the recipe has two edge-case sections, it also describes the three behaviours you identified. Each was verified against local HTML tables in real Chrome:

   • Removing a middle cell shifts later values to earlier columns; retaining an empty <td> preserves their positions.
   • Cells beyond the header count are ignored.
   • Without <thead>, the first row containing <th> supplies the header keys.

2. The counts refer to different test runs. The earlier count of 6 was for the combined test class only. The later count of 8 includes two standalone tests for short rows and <th>-only group rows. All 8 passed when run together.

3. The 8 tests cover:

   • Short rows independently.
   • <th>-only group rows independently.
   • Normal row reading, lookup by column value, and cell assertion.
   • Missing middle cells compared with explicitly empty cells.
   • Extra cells beyond the header count.
   • Header selection without <thead>.
   • Short rows and group headings together.
   • Rejection of an incorrect expected cell value.

I’ll update the PR description to make the coverage and counts explicit. We’ll also revise the recipe structure and reduce the repeated instructions. Thank you for pointing these out.

[May-Mei6]

I’ve restructured the main example as a titled test class, following the existing recipes such as handle-iframes.md, and added a Deeper reference link. I’ve also noted the template in the PR description.

[May-Mei6]

The Closes #123 line has been removed from this description for now, so that this internal PR doesn’t show up on the upstream issue. It will be added back in the upstream PR.

[crystalmountain666]

Looks good. These changes made the files easier to understand.

[Lisr-iiil]

The upstream PR is now open: seleniumboot#191

Thanks everyone for reviewing and helping improve the recipe. I’m closing this internal review PR now that the upstream PR is available.