Skip to content

867 pie chart block#1295

Merged
memisrose merged 13 commits intodevfrom
867-pie-chart-block
Jun 17, 2025
Merged

867 pie chart block#1295
memisrose merged 13 commits intodevfrom
867-pie-chart-block

Conversation

@RNubla
Copy link
Copy Markdown
Contributor

@RNubla RNubla commented Jun 12, 2025

Description

  • created test cases for PieChart and implemented customRenderHook so that useBlock hooks can be observe for changes made

Changes Made

  • create CustomHookRender
  • create test cases for PieChart

How to Test

  1. Steps to reproduce/test the behavior
  2. Expected outcomes

Notes

@RNubla RNubla requested a review from a team as a code owner June 12, 2025 15:36
@RNubla RNubla added the Test Used for tickets related to FE testing label Jun 12, 2025
@RNubla RNubla linked an issue Jun 12, 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

867 pie chart block


User description

Description

  • created test cases for PieChart and implemented customRenderHook so that useBlock hooks can be observe for changes made

Changes Made

  • create CustomHookRender
  • create test cases for PieChart

How to Test

  1. Steps to reproduce/test the behavior
  2. Expected outcomes

Notes


PR Type

Tests


Description

  • Add tests for PieChartBlock hook & rendering

  • Add donut toggle tests for PieChartBlock

  • Add tests for ProgressBlock hook & rendering

  • Enhance testing utils with renderHook support


Changes walkthrough 📝

Relevant files
Tests
PieChartBlock.spec.tsx
PieChartBlock spec tests added                                                     

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

  • Added unit tests for PieChartBlock with Vitest
  • Tested useBlock and useBlockSettings hooks
  • Verified chart rendering and ToggleDonut interaction
  • Asserted default series data and radius updates
  • +297/-0 
    ProgressBlock.spec.tsx
    ProgressBlock hook & render tests                                               

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

  • Imported renderHook, act, and useBlock
  • Added useBlock hook test for ProgressBlock
  • Verified ProgressBlock rendering and hidden state
  • +27/-5   
    Configuration changes
    index.tsx
    Enhance testing utils with hook support                                   

    libs/renderer/src/testing/utils/index.tsx

  • Exported custom renderHook and render functions
  • Introduced MockHookProvider for hook context
  • Refactored customRender wrapper logic
  • +107/-41

    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: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Brittle selector

    The test selects the donut toggle checkbox using a hidden role-based query, which is fragile and may break with markup changes. Consider adding an accessible label or data-testid to make the selector more reliable.

    /**
     * BUG
    	The label element associated with checkbox inputs must have an htmlFor attribute that corresponds to the id of the checkbox. 
    	This ensures proper accessibility support and allows assistive technologies to correctly associate labels with form elements.
     */
    const toggleSwitch = screen.getByRole("checkbox", { hidden: true }); //workaround for getting checkbox
    
    // screen.debug();
    expect(toggleSwitch).not.toBeChecked();
    fireEvent.click(toggleSwitch);
    expect(toggleSwitch).toBeChecked();
    expect(result.current.data.option.series[0].radius).toEqual(["20%", "50%"]);
    Debug logging

    There are console.log statements in tests, which can clutter test output. Remove or mock these logs to keep tests clean and deterministic.

    console.log({ result });
    
    expect(result.current).toBeDefined();
    Hook provider wrapper

    The customRenderHook wrapper injects both and children, but the provider hierarchy and initialProps usage may lead to unexpected hook context. Verify that useBlock and useBlockSettings hooks receive the correct context.

    const customRenderHook = <TProps, TResult>(
    	callback: (props: TProps) => TResult,
    	options?: CustomHookRenderOptions<TProps>,
    ) => {
    	const { blocks, queryConfig, renderEngineId, customChildren, ...hookOptions } = options;
    	// console.log({callback: callback})
    	return renderHook(callback, {
    		wrapper: ({ children, ...props }) => (
    			<MockHookProvider
    				{...props}
    				blocks={blocks}
    				queryConfig={queryConfig}
    				renderEngineId={renderEngineId}
    			>
    				{children}
    				{customChildren && customChildren}
    			</MockHookProvider>
    		),

    @QodoAI-Agent
    Copy link
    Copy Markdown

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Impact
    General
    Fix ToggleDonut import typo

    There's a typo in the imported component name. Correct ToogleDonut to ToggleDonut so
    the test imports the right component.

    libs/renderer/src/testing/block-defaults/PieChartBlock.spec.tsx [9]

    -import { ToogleDonut } from "@/components/block-defaults/echart-visualization-block/variant/pie-chart/ToggleDonut";
    +import { ToggleDonut } from "@/components/block-defaults/echart-visualization-block/variant/pie-chart/ToggleDonut";
    Suggestion importance[1-10]: 8

    __

    Why: The test uses <ToogleDonut> but the actual component export is ToggleDonut; correcting this prevents import failures and broken tests.

    Medium
    Simplify hook render wrapper

    Avoid spreading unexpected props into your provider wrapper. Use the captured
    blocks, queryConfig, and renderEngineId directly and accept only children in the
    wrapper signature.

    libs/renderer/src/testing/utils/index.tsx [110-120]

    -wrapper: ({ children, ...props }) => (
    +wrapper: ({ children }) => (
       <MockHookProvider
    -    {...props}
         blocks={blocks}
         queryConfig={queryConfig}
         renderEngineId={renderEngineId}
       >
         {children}
    -    {customChildren && customChildren}
    +    {customChildren}
       </MockHookProvider>
     ),
    Suggestion importance[1-10]: 4

    __

    Why: Restricting the wrapper signature to only children and using captured props streamlines the code, but this is a minor stylistic improvement.

    Low
    Possible issue
    Use boolean for show flag

    The show flag is declared as a string but should be a boolean to match ECharts'
    configuration types. Convert "true" to true for proper rendering behavior.

    libs/renderer/src/testing/block-defaults/PieChartBlock.spec.tsx [153]

    -show: "true",
    +show: true,
    Suggestion importance[1-10]: 5

    __

    Why: Changing show from a string to a boolean corrects the type and aligns with ECharts’ expected configuration, preventing potential rendering issues.

    Low

    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 0b26e7e into dev Jun 17, 2025
    4 checks passed
    @memisrose memisrose deleted the 867-pie-chart-block branch June 17, 2025 19:18
    @github-actions
    Copy link
    Copy Markdown

    @CodiumAI-Agent /update_changelog

    @QodoAI-Agent
    Copy link
    Copy Markdown

    Changelog updates: 🔄

    2025-06-17 *

    Added

    • custom hook render support in testing utils
    • PieChart block tests for rendering, settings, and donut toggle

    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.

    Pie Chart Block

    3 participants