Skip to content

Add a Window Timeout Field in the Command Editor, for Commands Marked as Opening a Window#1830

Merged
toddtarsi merged 1 commit into
SeleniumHQ:trunkfrom
AlexGarrity:window-timeout-field
May 18, 2024
Merged

Add a Window Timeout Field in the Command Editor, for Commands Marked as Opening a Window#1830
toddtarsi merged 1 commit into
SeleniumHQ:trunkfrom
AlexGarrity:window-timeout-field

Conversation

@AlexGarrity

@AlexGarrity AlexGarrity commented May 14, 2024 •
edited by qodo-code-review Bot
Loading

Copy link
Copy Markdown
Contributor

User description

Description

Adds a text field to the command editor that allows you to specify the window timeout if a command is flagged as opening a window.

Motivation and Context

(Copied from Issue #1829)
I use Selenium IDE to test a website which occasionally has quite long page load times. As a consequence of this, the default 2 second timeout on opening a window is too short, and frequently causes tests to fail when they're actually fine. It's possible to change this behaviour by editing the .side file itself, but it's not exactly the cleanest way of doing it.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement

Description

  • Added a new text field in the command editor for specifying a timeout when commands are flagged to open a new window.
  • Updated English and Chinese localizations to include descriptions for the new window timeout field.
  • Modified the command editor to conditionally display the new window timeout field based on command properties.

Changes walkthrough 📝

Relevant files
Enhancement
index.ts
Add English Localization for Window Timeout                           

packages/selenium-ide/src/browser/I18N/en/index.ts

  • Added new entries for 'windowTimeout' and 'windowTimeoutNote' to
    support the new window timeout functionality.
    		•  +2/-0     
    index.ts
    Add Chinese Localization for Window Timeout                           

    packages/selenium-ide/src/browser/I18N/zh/index.ts

  • Added new entries for 'windowTimeout' and 'windowTimeoutNote' in
    Chinese to support the new window timeout functionality.
    		•  +2/-0     
    TestCommandEditor.tsx
    Implement Window Timeout Field in Command Editor                 

    packages/selenium-ide/src/browser/windows/ProjectEditor/tabs/Tests/TestCommandEditor.tsx

  • Added a new text field for specifying window timeout when a command
    opens a new window.
  • This field only appears if the command is flagged to open a window.
    		•  +26/-12 
    types.ts
    Update Command Field Types to Include Window Timeout         

    packages/selenium-ide/src/browser/windows/ProjectEditor/tabs/Tests/types.ts

  • Updated the 'CommandFieldProps' interface to include 'windowTimeout'
    as a possible field name.
    		•  +1/-1     

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

    @CLAassistant

    CLAassistant commented May 14, 2024 •
    edited
    Loading

    Copy link
    Copy Markdown

    CLA assistant check
    All committers have signed the CLA.

    @qodo-code-review

    qodo-code-review Bot commented May 14, 2024

    Copy link
    Copy Markdown

    PR Description updated to latest commit (adcd7c4)

    @qodo-code-review

    qodo-code-review Bot commented May 14, 2024

    Copy link
    Copy Markdown

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are straightforward and localized to specific files related to UI and internationalization. The logic is simple and mainly involves adding new fields and labels.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Input Validation: Ensure that the 'windowTimeout' field properly validates the input to prevent non-numeric values or negative numbers, which could cause runtime errors or unexpected behavior.

    🔒 Security concerns

    No

    @qodo-code-review

    qodo-code-review Bot commented May 14, 2024

    Copy link
    Copy Markdown

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Add a type definition for the 'windowTimeout' field

    Include a type definition for the windowTimeout field in the CommandFieldProps interface
    to ensure type safety and consistency across the application.

    packages/selenium-ide/src/browser/windows/ProjectEditor/tabs/Tests/types.ts [20]

    -fieldName: 'comment' | 'windowHandleName' | 'windowTimeout' | LocatorFields
+fieldName: 'comment' | 'windowHandleName' | 'windowTimeout' | LocatorFields;
+windowTimeout?: number;  // Specify that windowTimeout should be a number
    Suggestion importance[1-10]: 8

    Why: This suggestion correctly identifies the need for type safety in TypeScript and targets the new code accurately. Ensuring type consistency is crucial for maintainability and error prevention.

    		8
    Enhancement
    Add a default value for the 'windowTimeout' field

    Consider adding a default value for the windowTimeout field to ensure that there is a
    fallback value if the timeout is not explicitly set by the user. This can help prevent
    runtime errors or unexpected behavior when the timeout value is undefined.

    packages/selenium-ide/src/browser/windows/ProjectEditor/tabs/Tests/TestCommandEditor.tsx [86-97]

     <CommandTextField
   command={correctedCommand}
   {...props}
   fieldName={
     intl.formatMessage({
       id: languageMap.testCore.windowTimeout,
     }) as 'windowTimeout'
   }
   note={intl.formatMessage({
     id: languageMap.testCore.windowTimeoutNote,
   })}
+  defaultValue="3000"  // Default to 3000 milliseconds
   />
    Suggestion importance[1-10]: 7

    Why: Adding a default value for 'windowTimeout' is a practical enhancement to prevent errors when the timeout is not set. This suggestion correctly targets the new code and improves usability.

    		7
    Verify translation accuracy for new keys in the Chinese language file

    Verify the translation accuracy for windowTimeout and windowTimeoutNote in the Chinese
    language file to ensure the translations are clear and accurate for the end-users.

    packages/selenium-ide/src/browser/I18N/zh/index.ts [186-187]

    +// Example: Ensure the translation conveys the correct technical meaning
 windowTimeout: '窗口超时',
 windowTimeoutNote: '等待窗口打开的时间量（以毫秒为单位）',
    Suggestion importance[1-10]: 5

    Why: Ensuring translation accuracy is important, but the suggestion lacks specific actionable improvements and does not provide a method to verify accuracy. It's more of a general reminder than a concrete code improvement.

    		5
    Maintainability
    Add new keys to other language files for internationalization

    Ensure that the new keys windowTimeout and windowTimeoutNote are also added to other
    language files to maintain internationalization support across different languages.

    packages/selenium-ide/src/browser/I18N/en/index.ts [180-181]

    +// Ensure these keys are added to other language files like 'de', 'fr', etc.
 windowTimeout: 'Window Timeout',
 windowTimeoutNote: 'The amount of time to wait for the window to open (in milliseconds)',
    Suggestion importance[1-10]: 6

    Why: The suggestion to add new keys to other language files is valid for maintaining internationalization. However, the PR already includes updates to another language file (Chinese), so the suggestion is somewhat redundant but still useful for other languages not shown.

    		6

    toddtarsi
    toddtarsi approved these changes May 18, 2024
    View reviewed changes

    @toddtarsi toddtarsi left a comment

    Copy link
    Copy Markdown
    Contributor

    Choose a reason for hiding this comment

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

    LGTM 👍 Merging this now and will increment later tonight.

    I absolutely appreciate this, thank you. There may be stuff to do here in the side-runtime to connect this fully. Would you like me to examine whats going on there?

    @toddtarsi toddtarsi merged commit 21f4d1a into SeleniumHQ:trunk May 18, 2024
    @AlexGarrity

    AlexGarrity commented May 18, 2024

    Copy link
    Copy Markdown
    Contributor Author

    I don't mind taking a look at it and seeing if anything needs connecting up - I think Intellisense might have pointed one or two things out so I'll start there

    @toddtarsi

    toddtarsi commented May 18, 2024

    Copy link
    Copy Markdown
    Contributor

    🪨 ⭐

    @AlexGarrity AlexGarrity deleted the window-timeout-field branch May 20, 2024 13:06
    @AlexGarrity AlexGarrity restored the window-timeout-field branch May 20, 2024 13:11
    @toddtarsi

    toddtarsi commented May 20, 2024

    Copy link
    Copy Markdown
    Contributor

    @AlexGarrity - Just wanted to let you know we cut beta v12 this morning, which has your window timeout changes. Nice work there again!

    @AlexGarrity

    AlexGarrity commented May 20, 2024

    Copy link
    Copy Markdown
    Contributor Author

    @toddtarsi Happy to hear it. Here's hoping that someone else finds it as useful as I have so far.

    I've had a look through side-runtime and -api but it looks to be pretty plug-and-play - the framework was there already, just not the UI. If you have any code areas in mind, however, then I'd be happy to take a look at them?

    @toddtarsi

    toddtarsi commented May 21, 2024

    Copy link
    Copy Markdown
    Contributor

    That's wonderful! I actually had no idea about if that field was getting picked off correctly honestly, nice job bringing that back fully then! 🥂

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

    Reviewers

    @toddtarsi toddtarsi toddtarsi approved these changes

    Assignees

    No one assigned

    Labels

    enhancement Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    3 participants

    @AlexGarrity @CLAassistant @toddtarsi