Skip to content

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented May 23, 2025

User description

🔗 Related Issues

💥 What does this PR do?

Clear few warning from component tests of Grid UI due to:

  • Missing mock

console.warn
No more mocked responses for the query: query GetSessions

  • Missing prop
console.error
      Warning: Each child in a list should have a unique "key" prop.
      
      Check the render method of `Overview`. See https://reactjs.org/link/warning-keys for more information.

https://github.com/SeleniumHQ/selenium/actions/runs/15211726088/job/42787262347

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)
  • New feature (non-breaking change which adds functionality and tests!)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR Type

Bug fix, Tests


Description

  • Fix missing "key" prop warning in Overview's sort menu.

  • Add missing GraphQL session mocks to Overview tests.

  • Update VNC session test data and assertions for accuracy.

  • Enhance test coverage for error and empty states in Overview.


Changes walkthrough 📝

Relevant files
Bug fix
Overview.tsx
Add missing "key" prop and minor cleanup in Overview         

javascript/grid-ui/src/screens/Overview/Overview.tsx

  • Added missing "key" prop to MenuItem in sort dropdown.
  • Minor formatting and whitespace adjustments.
  • +4/-4     
    Tests
    Node.test.tsx
    Update Node VNC session tests and assertions                         

    javascript/grid-ui/src/tests/components/Node.test.tsx

  • Updated VNC session test data to use realistic URLs.
  • Improved assertions for VNC URL transformation.
  • Enhanced test clarity and formatting.
  • +18/-20 
    Overview.test.tsx
    Add missing session mocks and enhance Overview tests         

    javascript/grid-ui/src/tests/components/Overview.test.tsx

  • Added missing mocks for GRID_SESSIONS_QUERY to prevent warnings.
  • Expanded mock session data with additional fields.
  • Improved test coverage for error and empty states.
  • +70/-3   

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @selenium-ci selenium-ci added the B-grid Everything grid and server related label May 23, 2025
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    5678 - Not compliant

    Non-compliant requirements:

    • Fix "ConnectFailure (Connection refused)" error when instantiating ChromeDriver multiple times

    Requires further human verification:

    • This PR appears to be focused on fixing UI test warnings in Grid UI, not addressing the ChromeDriver connection issue

    1234 - Not compliant

    Non-compliant requirements:

    • Fix issue where Selenium 2.48 doesn't trigger JavaScript in link's href on click()

    Requires further human verification:

    • This PR appears to be focused on fixing UI test warnings in Grid UI, not addressing the Firefox JavaScript click issue
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Test Coverage

    While the PR adds mocks for GRID_SESSIONS_QUERY to prevent warnings, verify that the test coverage is comprehensive for all possible states and edge cases in the Overview component.

    const mocks = [
      {
        request: {
          query: NODES_QUERY
        },
        result: {
          data: mockNodesData
        }
      },
      {
        request: {
          query: GRID_SESSIONS_QUERY
        },
        result: {
          data: mockSessionsData
        }
      },
      {
        request: {
          query: GRID_SESSIONS_QUERY
        },
        result: {
          data: mockSessionsData
        }
      },
    
    VNC URL Transformation

    The VNC URL transformation test has been updated with more specific assertions. Verify that this correctly reflects the actual behavior of the application when handling VNC URLs.

    it('correctly transforms VNC URL for WebSocket connection', async () => {
      const origin = 'https://grid.example.com'
      render(<Node node={node} sessions={[sessionWithVnc]} origin={origin} />)
    
      const user = userEvent.setup()
      await user.click(screen.getByTestId('VideocamIcon'))
    
      const liveView = screen.getByTestId('mock-live-view')
      const url = liveView.getAttribute('data-url')
    
      expect(url).toBe('wss://grid.example.com/session/4d31d065-6cb9-4545-ac8d-d70ed5f5168b/se/vnc')
    })
    

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Improve test resilience

    The test is making an assumption about the exact URL format, but it should
    verify the URL transformation logic instead. This makes the test more resilient
    to changes in the VNC URL structure while still validating the protocol
    conversion.

    javascript/grid-ui/src/tests/components/Node.test.tsx [156]

    -expect(url).toBe('wss://grid.example.com/session/4d31d065-6cb9-4545-ac8d-d70ed5f5168b/se/vnc')
    +const originalUrl = 'ws://172.17.0.2:4444/session/4d31d065-6cb9-4545-ac8d-d70ed5f5168b/se/vnc'
    +const expectedUrl = originalUrl.replace('ws://', 'wss://').replace('172.17.0.2:4444', 'grid.example.com')
    +expect(url).toBe(expectedUrl)
    • Apply / Chat
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion makes the test more robust by dynamically constructing the expected URL, reducing brittleness to changes in the VNC URL structure. However, the current explicit check is also valid if the URL format is stable, so this is a minor maintainability improvement rather than a critical fix.

    Low
    Remove duplicate mock definitions

    There are duplicate identical mock definitions for the GRID_SESSIONS_QUERY. This
    redundancy can cause confusion and maintenance issues. Remove one of the
    duplicate mocks to keep the test setup clean.

    javascript/grid-ui/src/tests/components/Overview.test.tsx [148-163]

    -{
    -  request: {
    -    query: GRID_SESSIONS_QUERY
    -  },
    -  result: {
    -    data: mockSessionsData
    -  }
    -},
     {
       request: {
         query: GRID_SESSIONS_QUERY
       },
       result: {
         data: mockSessionsData
       }
     },
    • Apply / Chat
    Suggestion importance[1-10]: 5

    __

    Why: Removing duplicate mocks improves test clarity and maintainability, but it is a minor cleanup that does not affect test correctness or functionality.

    Low
    Learned
    best practice
    Add unique key to list items

    When rendering lists of elements in React, each element should have a unique
    "key" prop to help React identify which items have changed. Adding the key prop
    improves rendering performance and prevents warnings.

    javascript/grid-ui/src/screens/Overview/Overview.tsx [193-195]

    -<MenuItem value={key}>
    +<MenuItem key={key} value={key}>
       {sortPropertiesLabel[key]}
     </MenuItem>

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 6

    __

    Why:
    Relevant best practice - Add a unique key prop to elements in a list to prevent React warnings and improve rendering performance

    Low
    • More

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

    Labels

    B-grid Everything grid and server related Review effort 2/5

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants