Skip to content

Time Picker Block #847#1119

Merged
memisrose merged 7 commits intodevfrom
time-picker-block-test
Jul 11, 2025
Merged

Time Picker Block #847#1119
memisrose merged 7 commits intodevfrom
time-picker-block-test

Conversation

@alaleung
Copy link
Copy Markdown
Contributor

Description

Time Picker Block
#847

@alaleung alaleung requested review from johbaxter and memisrose May 13, 2025 13:49
@alaleung alaleung requested a review from a team as a code owner May 13, 2025 13:49
@github-actions
Copy link
Copy Markdown

@CodiumAI-Agent /describe

@QodoAI-Agent
Copy link
Copy Markdown

Title

Time Picker Block #847


User description

Description

Time Picker Block
#847


PR Type

Tests


Description

  • Add tests for TimePickerBlock rendering

  • Verify initial and value-based displays

  • Simulate user interactions in time picker

  • Assert selected time updates correctly


Changes walkthrough 📝

Relevant files
Tests
TimePickerBlock.spec.tsx
New TimePickerBlock spec tests                                                     

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

  • Created spec file for TimePickerBlock
  • Added render test with mocked provider
  • Added value rendering and interaction test
  • Assert hours, minutes, AM/PM selection
  • +130/-0 

    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:

    🎫 Ticket compliance analysis 🔶

    847 - Partially compliant

    Compliant requirements:

    • Test default rendering of TimePickerBlock with no initial value
    • Test rendering of TimePickerBlock with a preset time value
    • Test interactive selection of hour, minute, and AM/PM in the picker dialog
    • Test presence of dialog role and OK button

    Non-compliant requirements:

    • Test clearable functionality (clear button behavior)
    • Test placeholder text behavior

    Requires further human verification:

    • Verify that the test assertions match the actual UI markup and localization settings
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Value Assertion Method

    Using inputElement.querySelector("[value='...']") to check the input’s value is unreliable; prefer inputElement.value or testing-library queries like toHaveValue.

        expect(inputElement.querySelector("[value='']")).toBeNull();
        expect(screen.getByText("Select Time")).toBeInTheDocument();
    });
    
    it("renders time value correctly", async () => {
        const { container } = render(<TimePickerBlock id="time-picker2" />, {
            blocks: blocks,
        });
    
        const element = container.querySelector("[data-block='time-picker2']");
        const inputElement = container.querySelector("input");
        const buttonElement = container.querySelector("button");
    
        expect(element).toBeInTheDocument();
        expect(inputElement).toBeInTheDocument();
        expect(inputElement.querySelector("[value='09:25 am']")).toBeNull();
    Incorrect Matcher

    The tests use .equals on expectations, which is not a Vitest matcher. Use .toEqual, .toBe, or another built-in matcher.

            expect(textElements.length).equals(2);
        } else {
            expect(screen.getByText(hourValue)).toBeInTheDocument();
        }
        const minuteValue = (i - 1) * 5;
        if (i > 3) {
            expect(screen.getByText(minuteValue)).toBeInTheDocument();
        }
    }
    expect(screen.getByText("00")).toBeInTheDocument();
    expect(screen.getByText("AM")).toBeInTheDocument();
    expect(screen.getByText("PM")).toBeInTheDocument();
    expect(screen.getByText("OK")).toBeInTheDocument();
    let selectedElements = timePickerElement.querySelectorAll(
        "[aria-selected='true']",
    );
    expect(selectedElements[0].textContent).equal("09");
    expect(selectedElements[1].textContent).equal("25");
    expect(selectedElements[2].textContent).equal("AM");
    Brittle Loop Logic

    The hour/minute selection loop makes hardcoded assumptions about duplicate entries and visibility that may break if the UI structure or localization changes.

    for (let i = 1; i < 13; i++) {
        const hourValue = i < 10 ? `0${i}` : `${i}`;
        if (i === 5 || i === 10) {
            const textElements = screen.getAllByText(hourValue);
            expect(textElements).not.toBeNull();
            expect(textElements.length).equals(2);
        } else {
            expect(screen.getByText(hourValue)).toBeInTheDocument();
        }
        const minuteValue = (i - 1) * 5;
        if (i > 3) {
            expect(screen.getByText(minuteValue)).toBeInTheDocument();
        }
    }

    @QodoAI-Agent
    Copy link
    Copy Markdown

    QodoAI-Agent commented May 13, 2025

    PR Code Suggestions ✨

    Latest suggestions up to e977980

    CategorySuggestion                                                                                                                                    Impact
    General
    Assert input shows correct time

    To verify the rendered time value, assert the input’s value property directly with
    the proper matcher instead of querying a child element. This ensures correct
    checking of the displayed time.

    libs/renderer/src/testing/block-defaults/TimePickerBlock.spec.tsx [90]

    -expect(inputElement.querySelector("[value='09:25 am']")).toBeNull();
    +expect(inputElement).toHaveValue('09:25 am');
    Suggestion importance[1-10]: 8

    __

    Why: This fix corrects the test by asserting the actual displayed time with toHaveValue rather than expecting a null selector, improving test accuracy.

    Medium
    Use toHaveValue for empty input

    Instead of querying a child element for the input’s value, use the dedicated Jest
    DOM matcher to assert the input’s value directly. This makes the intent clearer and
    avoids misusing querySelector on an input element.

    libs/renderer/src/testing/block-defaults/TimePickerBlock.spec.tsx [75]

    -expect(inputElement.querySelector("[value='']")).toBeNull();
    +expect(inputElement).toHaveValue('');
    Suggestion importance[1-10]: 6

    __

    Why: Using toHaveValue directly makes the intent clearer and correctly asserts the input’s value instead of misusing querySelector.

    Low
    Possible issue
    Replace .equals with .toBe

    The Vitest/Jest expect API does not include .equals; replace it with .toBe for
    strict equality assertions. This fixes matcher errors and clarifies the expectation.

    libs/renderer/src/testing/block-defaults/TimePickerBlock.spec.tsx [100]

    -expect(textElements.length).equals(2);
    +expect(textElements.length).toBe(2);
    Suggestion importance[1-10]: 7

    __

    Why: Changing .equals to .toBe uses a valid Jest/Vitest matcher and prevents test failures due to unsupported methods.

    Medium

    Previous suggestions

    Suggestions up to commit 675b5a6
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix array length assertion

    The .equals matcher is not valid in Vitest/Jest. Use .toHaveLength(2) to accurately
    assert the number of elements in the array.

    libs/renderer/src/testing/block-defaults/TimePickerBlock.spec.tsx [100]

    -expect(textElements.length).equals(2);
    +expect(textElements).toHaveLength(2);
    Suggestion importance[1-10]: 8

    __

    Why: Replacing the invalid .equals matcher with toHaveLength(2) uses the correct Vitest/Jest API and fixes a potential test failure.

    Medium
    Correct string equality assertion

    Replace the invalid .equal matcher with .toBe("09") for a correct strict equality
    comparison of the text content.

    libs/renderer/src/testing/block-defaults/TimePickerBlock.spec.tsx [116]

    -expect(selectedElements[0].textContent).equal("09");
    +expect(selectedElements[0].textContent).toBe("09");
    Suggestion importance[1-10]: 8

    __

    Why: The .equal matcher isn’t valid in Vitest; using toBe("09") ensures a proper strict equality check on the text content.

    Medium
    General
    Use proper value assertion

    Use jest-dom's toHaveValue matcher to assert the input's value instead of querying
    for a child element. This makes the test clearer and correctly checks the input's
    value.

    libs/renderer/src/testing/block-defaults/TimePickerBlock.spec.tsx [75]

    -expect(inputElement.querySelector("[value='']")).toBeNull();
    +expect(inputElement).toHaveValue('');
    Suggestion importance[1-10]: 6

    __

    Why: Using toHaveValue('') with inputElement makes the test clearer and directly checks the input’s value instead of querying a child element.

    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.

    issue rendering the component

    @memisrose memisrose marked this pull request as draft May 19, 2025 17:06
    @memisrose memisrose marked this pull request as ready for review June 17, 2025 19:43
    @alaleung
    Copy link
    Copy Markdown
    Contributor Author

    issue rendering the component

    fixed

    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

    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
    the second test is failing

    @alaleung
    Copy link
    Copy Markdown
    Contributor Author

    image the second test is failing

    Fixed. This is a time zone issue

    @alaleung
    Copy link
    Copy Markdown
    Contributor Author

    image the second test is failing

    Fixed this is a time zone issue

    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.

    all tests pass with no issues

    @memisrose memisrose merged commit 6e3bd87 into dev Jul 11, 2025
    3 checks passed
    @memisrose memisrose deleted the time-picker-block-test branch July 11, 2025 17:21
    @github-actions
    Copy link
    Copy Markdown

    @CodiumAI-Agent /update_changelog

    @QodoAI-Agent
    Copy link
    Copy Markdown

    Changelog updates: 🔄

    2025-07-11 *

    Added

    • Added tests for the Time Picker Block UI component

    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

    None yet

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    3 participants