Skip to content
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

Updated Design Patterns And Development Strategies' Example Scripts #1949

Merged

Conversation

shbenzer
Copy link
Contributor

@shbenzer shbenzer commented Sep 18, 2024

User description

Design patterns and development strategies' example scripts were outdated, so I updated them to reflect the new changes.

Description

updated index.md in all languages with new code examples

Motivation and Context

Github updated https://github.com/SeleniumHQ/selenium/issues/new, and since Design patterns and development strategies uses the page for its example scripts, those scripts needed to be updated.

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

enhancement


Description

  • Updated the design patterns and development strategies example scripts to align with the new GitHub issues page.
  • Replaced outdated methods setSummary and enterDescription with new methods for setting various issue form fields.
  • Added new methods to handle additional fields in the issue form, such as setTitle, setWhatHappened, setHowToReproduce, setLogOutput, setOperatingSystem, setSeleniumVersion, setBrowserVersion, setDriverVersion, and setUsingGrid.
  • Changed the selector for the submit button to a more specific CSS selector.

Changes walkthrough 📝

Relevant files
Enhancement
design_strategies.en.md
Update issue form handling methods in English documentation

website_and_docs/content/documentation/test_practices/design_strategies.en.md

  • Updated method names and parameters for issue form fields.
  • Replaced setSummary and enterDescription with new methods.
  • Changed the submit button selector.
  • Added new methods for additional issue form fields.
  • +105/-14
    design_strategies.ja.md
    Update issue form handling methods in Japanese documentation

    website_and_docs/content/documentation/test_practices/design_strategies.ja.md

  • Updated method names and parameters for issue form fields.
  • Replaced setSummary and enterDescription with new methods.
  • Changed the submit button selector.
  • Added new methods for additional issue form fields.
  • +104/-14
    design_strategies.pt-br.md
    Update issue form handling methods in Portuguese documentation

    website_and_docs/content/documentation/test_practices/design_strategies.pt-br.md

  • Updated method names and parameters for issue form fields.
  • Replaced setSummary and enterDescription with new methods.
  • Changed the submit button selector.
  • Added new methods for additional issue form fields.
  • +104/-14
    design_strategies.zh-cn.md
    Update issue form handling methods in Chinese documentation

    website_and_docs/content/documentation/test_practices/design_strategies.zh-cn.md

  • Updated method names and parameters for issue form fields.
  • Replaced setSummary and enterDescription with new methods.
  • Changed the submit button selector.
  • Added new methods for additional issue form fields.
  • +104/-14

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    netlify bot commented Sep 18, 2024

    👷 Deploy request for selenium-dev pending review.

    Visit the deploys page to approve it

    Name Link
    🔨 Latest commit 0c28974

    @codiumai-pr-agent-pro codiumai-pr-agent-pro bot added the enhancement New feature or request label Sep 18, 2024
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Possible Bug
    The setSeleniumVersion method is using logOutput instead of seleniumVersion as the parameter to be set.

    Code Smell
    The getStarted method is missing a semicolon after the click() call.

    Copy link
    Contributor

    codiumai-pr-agent-pro bot commented Sep 18, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Use correct parameter in method call to set the Selenium version

    In the setSeleniumVersion method, replace logOutput with seleniumVersion as the
    argument for clearAndType to correctly set the Selenium version.

    website_and_docs/content/documentation/test_practices/design_strategies.en.md [85-88]

     public void setSeleniumVersion(String seleniumVersion) {
       WebElement field = driver.findElement(By.id("issue_form_selenium-version"));
    -  clearAndType(field, logOutput);
    +  clearAndType(field, seleniumVersion);
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: This suggestion addresses a critical bug where the wrong variable is used, potentially leading to incorrect behavior. Correcting it ensures the method functions as intended.

    10
    Syntax
    ✅ Add missing semicolons to ensure proper Java syntax
    Suggestion Impact:The suggestion was implemented by adding semicolons to the end of both the field initialization and the field.click() statement, correcting the syntax.

    code diff:

    -    WebElement field = driver.findElement(By.xpath("//*[contains(text(), 'Get Started')]"))
    -    field.click()
    +    WebElement field = driver.findElement(By.xpath("//*[contains(text(), 'Get Started')]"));
    +    field.click();

    Add a semicolon at the end of the field.click() statement to ensure proper syntax.

    website_and_docs/content/documentation/test_practices/design_strategies.en.md [55-58]

     public void getStarted() {
    -  WebElement field = driver.findElement(By.xpath("//*[contains(text(), 'Get Started')]"))
    -  field.click()
    +  WebElement field = driver.findElement(By.xpath("//*[contains(text(), 'Get Started')]"));
    +  field.click();
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly identifies a missing semicolon, which is crucial for proper Java syntax. Adding it resolves a syntax error.

    9
    ✅ Remove extra closing parenthesis to fix syntax error
    Suggestion Impact:The extra closing parenthesis in the driver.findElement(By.id("issue_title")) call was removed, fixing the syntax error.

    code diff:

       public void setTitle(String title) {
         WebElement field = driver.findElement(By.id("issue_title")));
         clearAndType(field, title);

    Remove the extra closing parenthesis in the driver.findElement() call to fix the
    syntax error.

    website_and_docs/content/documentation/test_practices/design_strategies.en.md [60-63]

     public void setTitle(String title) {
    -  WebElement field = driver.findElement(By.id("issue_title")));
    +  WebElement field = driver.findElement(By.id("issue_title"));
       clearAndType(field, title);
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion accurately points out an extra parenthesis causing a syntax error, and its removal is necessary for correct code execution.

    9
    ✅ Add missing semicolons and use double quotes for string literals in Java
    Suggestion Impact:The commit added missing semicolons to the method calls in the demonstrateNestedLoadableComponents method, but did not change single quotes to double quotes for string literals.

    code diff:

    @@ -405,16 +405,16 @@
       public void demonstrateNestedLoadableComponents() {
         editIssue.get();
     
    -    editIssue.getStarted()
    -    editIssue.setTitle('Title')
    -    editIssue.setWhatHappened('What Happened')
    -    editIssue.setHowToReproduce('How to Reproduce')
    -    editIssue.setLogOutput('Log Output')
    -    editIssue.setOperatingSystem('Operating System')
    -    editIssue.setSeleniumVersion('Selenium Version')
    -    editIssue.setBrowserVersion('Browser Version')
    -    editIssue.setDriverVersion('Driver Version')
    -    editIssue.setUsingGrid('I Am Using Grid')
    +    editIssue.getStarted();
    +    editIssue.setTitle('Title');
    +    editIssue.setWhatHappened('What Happened');
    +    editIssue.setHowToReproduce('How to Reproduce');
    +    editIssue.setLogOutput('Log Output');
    +    editIssue.setOperatingSystem('Operating System');
    +    editIssue.setSeleniumVersion('Selenium Version');
    +    editIssue.setBrowserVersion('Browser Version');
    +    editIssue.setDriverVersion('Driver Version');
    +    editIssue.setUsingGrid('I Am Using Grid');

    Add semicolons to the method calls in the demonstrateNestedLoadableComponents method
    to ensure proper Java syntax.

    website_and_docs/content/documentation/test_practices/design_strategies.en.md [405-417]

     public void demonstrateNestedLoadableComponents() {
       editIssue.get();
     
    -  editIssue.getStarted()
    -  editIssue.setTitle('Title')
    -  editIssue.setWhatHappened('What Happened')
    -  editIssue.setHowToReproduce('How to Reproduce')
    -  editIssue.setLogOutput('Log Output')
    -  editIssue.setOperatingSystem('Operating System')
    -  editIssue.setSeleniumVersion('Selenium Version')
    -  editIssue.setBrowserVersion('Browser Version')
    -  editIssue.setDriverVersion('Driver Version')
    -  editIssue.setUsingGrid('I Am Using Grid')
    +  editIssue.getStarted();
    +  editIssue.setTitle("Title");
    +  editIssue.setWhatHappened("What Happened");
    +  editIssue.setHowToReproduce("How to Reproduce");
    +  editIssue.setLogOutput("Log Output");
    +  editIssue.setOperatingSystem("Operating System");
    +  editIssue.setSeleniumVersion("Selenium Version");
    +  editIssue.setBrowserVersion("Browser Version");
    +  editIssue.setDriverVersion("Driver Version");
    +  editIssue.setUsingGrid("I Am Using Grid");
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly adds missing semicolons, which are essential for Java syntax. It also improves code readability by using double quotes for string literals.

    8

    💡 Need additional feedback ? start a PR chat

    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 @shbenzer,

    Thank you for the PR.

    I understand the need to update the locators as the current ones are outdated.

    However, the additional changes seem to be related to issue template updates, which might not be necessary in this context. Including them could also make the code sample appear larger than required.

    It would be great if we could focus solely on updating the locators according to the template, while keeping the page objects unchanged.

    Please let me know your thoughts!

    Thanks,
    Sri

    @shbenzer
    Copy link
    Contributor Author

    I'm not 100% sure I understand what you mean.

    Are you saying to instead just change the current functions to be relevant to the new page, but not add functions for everything on the new page, so that we can keep the same number of functions and the example doesn't change significantly in length?

    @shbenzer
    Copy link
    Contributor Author

    @harsha509 would you mind expanding on your comments here?

    @harsha509
    Copy link
    Member

    I'm not 100% sure I understand what you mean.

    Are you saying to instead just change the current functions to be relevant to the new page, but not add functions for everything on the new page, so that we can keep the same number of functions and the example doesn't change significantly in length?

    Hi @shbenzer,

    Yes, we can proceed by fixing the existing issue rather than adding new functions.

    Additionally, there are a couple of things I do not understand. For example, the locator issue_form_operating-system is not present on the page SeleniumHQ Issue Submission. Could you please clarify this?

    Thank you.

    @shbenzer
    Copy link
    Contributor Author

    shbenzer commented Sep 30, 2024

    Yes, we can proceed by fixing the existing issue rather than adding new functions.

    @harsha509 Gotcha

    Additionally, there are a couple of things I do not understand. For example, the locator issue_form_operating-system is not present on the page SeleniumHQ Issue Submission. Could you please clarify this?

    The old code is based on what we access by clicking "Get Started" next to Bug Report. Since not only has SeleniumHQ Issue Submission changed to let you pick which issue you're creating, the page we originally utilized has also changed to have more and different fields ("what happened", "what operating system are you using", "log output", etc.). The issue_form_operating-system locator is for the "Operating System: What host operating system are you using to run Selenium?" form field.

    @harsha509
    Copy link
    Member

    Yes, we can proceed by fixing the existing issue rather than adding new functions.

    @harsha509 Gotcha

    Additionally, there are a couple of things I do not understand. For example, the locator issue_form_operating-system is not present on the page SeleniumHQ Issue Submission. Could you please clarify this?

    The old code is based on what we access by clicking "Get Started" next to Bug Report. Since not only has SeleniumHQ Issue Submission changed to let you pick which issue you're creating, the page we originally utilized has also changed to have more and different fields ("what happened", "what operating system are you using", "log output", etc.). The issue_form_operating-system locator is for the "Operating System: What host operating system are you using to run Selenium?" form field.

    Understood now!

    The new additions are from different pages and has different url's

    https://github.com/SeleniumHQ/selenium/issues
    https://github.com/SeleniumHQ/selenium/issues/new/choose
    https://github.com/SeleniumHQ/selenium/issues/new?assignees=&labels=I-defect%2Cneeds-triaging&projects=&template=bug-report.yml&title=%5B%F0%9F%90%9B+Bug%5D%3A+

    Since this is just an example for the page object of a single page (https://github.com/SeleniumHQ/selenium/issues/new), I think changes related to this page should be enough.

    @shbenzer
    Copy link
    Contributor Author

    shbenzer commented Oct 1, 2024

    Since this is just an example for the page object of a single page (https://github.com/SeleniumHQ/selenium/issues/new), I think changes related to this page should be enough.

    @harsha509 If that's the case, may I change the link from https://github.com/SeleniumHQ/selenium/issues/new to https://github.com/SeleniumHQ/selenium/issues/new?assignees=&labels=I-defect%2Cneeds-triaging&projects=&template=bug-report.yml&title=%5B%F0%9F%90%9B+Bug%5D%3A+?

    Otherwise I think I'll have to make more changes than you're recommending.

    @harsha509
    Copy link
    Member

    https://github.com/SeleniumHQ/selenium/issues
    https://github.com/SeleniumHQ/selenium/issues/new/choose
    https://github.com/SeleniumHQ/selenium/issues/new?assignees=&labels=I-defect%2Cneeds-triaging&projects=&template=bug-report.yml&title=%5B%F0%9F%90%9B+Bug%5D%3A+

    Since this is just an example for the page object of a single page (https://github.com/SeleniumHQ/selenium/issues/new), I think changes related to this page should be enough.

    @harsha509 If that's the case, may I change the link from https://github.com/SeleniumHQ/selenium/issues/new to https://github.com/SeleniumHQ/selenium/issues/new?assignees=&labels=I-defect%2Cneeds-triaging&projects=&template=bug-report.yml&title=%5B%F0%9F%90%9B+Bug%5D%3A+?

    Otherwise I think I'll have to make more changes than you're recommending.

    I thought to suggest the same initially,
    but , I think these should be treated as three separate pages, each with its own PageObject. It wouldn’t make sense to combine them into one class. So suggested the above.

    @shbenzer
    Copy link
    Contributor Author

    shbenzer commented Oct 1, 2024

    Just so I'm clear - would that mean the following changes:

    1. remove the setters (e.g. setSummary(), set Title(), etc.)
    2. replace the setters with getters for each link on New Issue

    This way we would only need one page object and can make the least amount of changes to the page.

    @harsha509
    Copy link
    Member

    Just so I'm clear - would that mean the following changes:

    1. remove the setters (e.g. setSummary(), set Title(), etc.)
    2. replace the setters with getters for each link on New Issue

    This way we would only need one page object and can make the least amount of changes to the page.

    Yes, that works too. The main goal is to help users understand the Page Object Model.

    @shbenzer
    Copy link
    Contributor Author

    shbenzer commented Oct 1, 2024

    Cool, I'll try to knock that out tonight or tomorrow if I can

    @shbenzer
    Copy link
    Contributor Author

    shbenzer commented Oct 1, 2024

    @harsha509 So I ran into a problem and created a new solution... let me know what you think

    In order to illustrate PageFactory's ability to locate elements with the same name/id of the field I had to shift the site to Bug Report so that the EditIssue class can still have the following:

    // By default the PageFactory will locate elements with the same name or id
    // as the field. Since the issue_title element has an id attribute of "issue_title"
    // we don't need any additional annotations.
    private WebElement issue_title;

    // But we'd prefer a different name in our code than "issue_body", so we use the
    // FindBy annotation to tell the PageFactory how to locate the element.
    @findby(id = "issue_body") private WebElement body;

    This is different than what we discussed. Since the only ids on the old page contained hyphens, I couldn't use them as variable names in java, plus they would have had to be the tabs to Pull Requests, Issues, etc. and wouldn't have really fit the example anyway.

    Let me know what you think since this does still increase the size of the example a tad. However, the example focuses on a single website for the page object model like page objects should.

    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.

    Thank you @shbenzer !

    @harsha509 harsha509 merged commit 30c9eae into SeleniumHQ:trunk Oct 8, 2024
    3 checks passed
    selenium-ci added a commit that referenced this pull request Oct 8, 2024
    …1949)[deploy site]
    
    * updated Design Patterns And Strategies for new Github Issues Page
    
    * missing semicolons
    
    * missing semicolons
    
    * missing semicolons
    
    * missing semicolons
    
    * missing semicolons
    
    * refactored to focus pageBody object on new issue site 30c9eae
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants