Skip to content

827 popover block test#1177

Merged
memisrose merged 11 commits intodevfrom
827-popover-block-test
Jul 16, 2025
Merged

827 popover block test#1177
memisrose merged 11 commits intodevfrom
827-popover-block-test

Conversation

@Mezzet
Copy link
Copy Markdown
Contributor

@Mezzet Mezzet commented May 20, 2025

Description

Adding Popover block tests

Changes Made

Added popover test file

How to Test

Create mock data
Run tests

Notes

@Mezzet Mezzet linked an issue May 20, 2025 that may be closed by this pull request
@github-actions
Copy link
Copy Markdown

@CodiumAI-Agent /describe

@QodoAI-Agent
Copy link
Copy Markdown

Title

827 popover block test


User description

Description

Adding Popover block tests

Changes Made

Added popover test file

How to Test

Create mock data
Run tests

Notes


PR Type

Tests


Description

  • Add unit tests for PopoverBlock

  • Verify popover opens on click trigger

  • Check default placeholder content

  • Ensure styled popover applies custom styles


Changes walkthrough 📝

Relevant files
Tests
Popover.spec.tsx
Add unit tests for PopoverBlock                                                   

libs/renderer/src/testing/block-defaults/Popover.spec.tsx

  • Create mock block definitions for popover scenarios
  • Render ContainerBlock as popover target
  • Test default popover open and placeholder text
  • Validate styled popover dimensions and content
  • +182/-0 
    Additional files
    pnpm-lock.yaml [link]   

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @github-actions
    Copy link
    Copy Markdown

    @CodiumAI-Agent /review

    @github-actions
    Copy link
    Copy Markdown

    @CodiumAI-Agent /improve

    @QodoAI-Agent
    Copy link
    Copy Markdown

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Isolated render

    The test’s click target is obtained from a separate render of ContainerBlock in beforeEach and not part of the PopoverBlock render under test. As a result, fireEvent.click on that target will not open the popover instance being rendered in each individual test. Consider rendering ContainerBlock and PopoverBlock together or querying the correct element within the same render scope.

    fireEvent.click(target);
    Style assertion

    The tests mix object-style and string-style assertions and include spaces in the rgb value, which may cause false negatives. Standardize to one assertion style (e.g., object with exact values) and ensure the rgb formatting matches the actual computed style.

    expect(element).toHaveStyle({ width: "200px", height: "100px" });
    
    expect(element).toHaveStyle("backgroundColor: rgb(108, 71, 71)");
    expect(element).toHaveStyle("border: 1px solid #000000");

    @QodoAI-Agent
    Copy link
    Copy Markdown

    QodoAI-Agent commented May 20, 2025

    PR Code Suggestions ✨

    Latest suggestions up to a418013

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    include container in render

    Adjust the render call to include the parent ContainerBlock in the same React tree
    so that the target element exists and receives click events. Merge both blocks into
    one render invocation.

    libs/renderer/src/testing/block-defaults/Popover.spec.tsx [142-148]

    -const { container } = render(<PopoverBlock id="popover" />, {
    -    blocks: {
    -        popover: {
    -            ...blocks["popover"],
    +const { container } = render(
    +    <>
    +        <ContainerBlock id="target-container" />
    +        <PopoverBlock id="popover" />
    +    </>,
    +    {
    +        blocks: {
    +            "target-container": {
    +                ...blocks["target-container"],
    +            },
    +            popover: {
    +                ...blocks["popover"],
    +            },
             },
    -    },
    -});
    +    }
    +);
    Suggestion importance[1-10]: 8

    __

    Why: Wrapping both ContainerBlock and PopoverBlock in the same render ensures the target element exists in the same DOM tree for click events, fixing a broken test setup.

    Medium
    General
    use object style assertion

    Use an object argument with camelCase property names in toHaveStyle for more
    reliable style assertions. This matches other style checks in the test.

    libs/renderer/src/testing/block-defaults/Popover.spec.tsx [177]

    -expect(element).toHaveStyle("backgroundColor: rgb(108, 71, 71)");
    +expect(element).toHaveStyle({ backgroundColor: "rgb(108, 71, 71)" });
    Suggestion importance[1-10]: 5

    __

    Why: Switching to an object in toHaveStyle improves consistency with other style checks but has minimal functional impact.

    Low

    Previous suggestions

    Suggestions up to commit dce2766
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Render popover with its container

    Render the popover inside the container block so the click target exists in the same
    DOM tree and query it from the same container. This ensures the click actually
    triggers the popover open logic.

    libs/renderer/src/testing/block-defaults/Popover.spec.tsx [142-151]

    -const { container } = render(<PopoverBlock id="popover" />, {
    +const { container } = render(<ContainerBlock id="target-container" />, {
         blocks: {
    -        popover: {
    -            ...blocks["popover"],
    +        "target-container": {
    +            ...blocks["target-container"],
    +            slots: {
    +                children: { children: ["popover"], name: "children" }
    +            }
             },
    +        popover: { ...blocks["popover"] }
         },
     });
    -// click to trigger popover
    +const target = container.querySelector("[data-block='target-container']");
     fireEvent.click(target);
    Suggestion importance[1-10]: 9

    __

    Why: The test currently renders the PopoverBlock separately from the container, causing the click on target to not be in the same DOM; wrapping in ContainerBlock fixes the click trigger context and corrects the test.

    High
    Render styled popover inside container

    Include the styled popover within the same container rendering so the click target
    is present, and query it from the returned container. This aligns the DOM contexts
    for trigger and popover.

    libs/renderer/src/testing/block-defaults/Popover.spec.tsx [160-171]

    -const { container } = render(<PopoverBlock id="styledPopover" />, {
    +const { container } = render(<ContainerBlock id="target-container" />, {
         blocks: {
    -        styledPopover: {
    -            ...blocks["styledPopover"],
    +        "target-container": {
    +            ...blocks["target-container"],
    +            slots: {
    +                children: { children: ["styledPopover"], name: "children" }
    +            }
             },
    -        helloText: {
    -            ...blocks["helloText"],
    -        },
    +        styledPopover: { ...blocks["styledPopover"] },
    +        helloText: { ...blocks["helloText"] }
         },
     });
    +const target = container.querySelector("[data-block='target-container']");
     fireEvent.click(target);
    Suggestion importance[1-10]: 9

    __

    Why: Similar to the first test, the styled popover is rendered outside its container, so including it in the ContainerBlock ensures the click correctly triggers popover display.

    High

    @Mezzet Mezzet marked this pull request as ready for review May 20, 2025 02:37
    @Mezzet Mezzet requested a review from a team as a code owner May 20, 2025 02:37
    @memisrose memisrose self-assigned this Jun 17, 2025
    @memisrose memisrose added the Test Used for tickets related to FE testing label Jun 17, 2025
    @memisrose memisrose self-requested a review June 17, 2025 20:14
    @memisrose memisrose removed their assignment Jun 17, 2025
    Copy link
    Copy Markdown
    Contributor

    @memisrose memisrose left a comment

    Choose a reason for hiding this comment

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

    image
    image

    The tests pass here, but there are errors.

    @memisrose
    Copy link
    Copy Markdown
    Contributor

    @Mezzet, I updated the branch, but am still seeing the issues above. Initially, this test passes, but the error appears when all tests are completed.

    image

    Copy link
    Copy Markdown
    Contributor

    @memisrose memisrose left a comment

    Choose a reason for hiding this comment

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

    image

    @memisrose memisrose merged commit 0ef2820 into dev Jul 16, 2025
    3 checks passed
    @memisrose memisrose deleted the 827-popover-block-test branch July 16, 2025 00:25
    @github-actions
    Copy link
    Copy Markdown

    @CodiumAI-Agent /update_changelog

    @QodoAI-Agent
    Copy link
    Copy Markdown

    Changelog updates: 🔄

    2025-07-16 *

    Added

    • Tests for the Popover block covering rendering, interaction, and style validation

    to commit the new content to the CHANGELOG.md file, please type:
    '/update_changelog --pr_update_changelog.push_changelog_changes=true'

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

    Labels

    Test Used for tickets related to FE testing

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    Popover Block

    3 participants