Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

docs: improve consistency and readability of the guide #2168

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kathyrollo wants to merge 4 commits into SeleniumHQ:trunk
base: trunk
Choose a base branch
Loading
from kathyrollo:patch-3

Conversation

@kathyrollo
Copy link
Contributor

@kathyrollo kathyrollo commented Feb 15, 2025 •
edited by qodo-merge-pro bot
Loading

User description

Hi @diemol , this is a refinement of my previous contributions for Page Component Objects (#1387, #1394).

Description

  • Add emphasis on the relationship between Page Objects and Page Component Objects.
  • Add a descriptive message to the error thrown, which informs the tester of the faulty strategy (predicate) passed.

Motivation and Context

  • The changes highlight Page Objects and Page Component Objects are used together, not one over the other. While this was previously implied, it was not obviously explicit.
  • A specific error message clearly identifies the error with the test rather than actual programming error, compared to a generic Exception message thrown.
  • Overall consistency of guide is improved, so the idea remains cohesive in all sections of the article.

Types of changes

  • Change to the site (I have double-checked the Netlify deployment, and my changes look good)
  • Code example added (and I also added the example to all translated languages)
  • Improved translation
  • Added new translation (and I also added a notice to each document missing translation)

Checklist

  • I have read the contributing document.
  • I have used hugo to render the site/docs locally and I am sure it works.

PR Type

Documentation

Description

  • Improved consistency and terminology in the Page Object Model guide.

  • Added emphasis on the relationship between Page Objects and Page Component Objects.

  • Enhanced error messages for better clarity during test failures.

  • Refined examples and explanations for better readability and understanding.

Changes walkthrough 📝

Relevant files
Documentation
page_object_models.en.md
Refined Page Object Model guide for clarity and consistency

website_and_docs/content/documentation/test_practices/encouraged/page_object_models.en.md

  • Replaced "Page Object" with "Page Object Model" for consistency.
  • Added clarifications on Page Component Objects and their relationship
    with Page Objects.
  • Improved error message for better debugging during test failures.
  • Enhanced explanations and examples for better readability and
    understanding.
    • +18/-15
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    Copy link

    netlify bot commented Feb 15, 2025 •
    edited
    Loading

    ✅ Deploy Preview for selenium-dev canceled.

    Name Link
    🔨 Latest commit b7faabc
    🔍 Latest deploy log https://app.netlify.com/projects/selenium-dev/deploys/67affd7c119f9c0008a7f6dc

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Message

    The new error message "Product not found" is generic and could be more descriptive. Consider including details about the failed condition/predicate to help with debugging.

     
    .orElseThrow(() -> new RuntimeException("Product not found")); // Error thrown during actual test run

    Copy link
    Contributor

    qodo-merge-pro bot commented Feb 15, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion Impact
    General
    Improve error message with context

    The error message in the RuntimeException should include more context about the
    search condition to help with debugging. Include details about the filter
    condition that failed to match.

    website_and_docs/content/documentation/test_practices/encouraged/page_object_models.en.md [285] 

    -.orElseThrow(() -> new RuntimeException("Product not found")); // Error thrown during actual test run
+.orElseThrow(() -> new RuntimeException("Product not found. Filter condition: " + condition.toString())); // Error thrown during actual test run
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: Adding the filter condition to the error message would significantly improve debugging capabilities by providing more context about why the product search failed, making it easier to identify and fix issues during test failures.

    		Medium

    Copy link
    Member

    @harsha509 harsha509 left a comment

    Choose a reason for hiding this comment

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

    HI @kathyrollo ,

    Thank you for the PR.

    Could you please add/update similar changes in other translated pages too( pt-br.md,
    zh-cn.md, ja.md), which are other than in en(english) ?

    Thanks,
    Sri

    kathyrollo reacted with thumbs up emoji
    Copy link
    Contributor Author

    Cc: @diemol

    Hi @harsha509 , sorry it took a bit. I have updated the relevant files.

    Thanks for reviewing,
    Kat

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

    Reviewers

    @harsha509 harsha509 Awaiting requested review from harsha509

    Requested changes must be addressed to merge this pull request.

    Assignees

    No one assigned

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    AltStyle によって変換されたページ (->オリジナル) /