CONSOLE-5041: Replace testHook with renderHook from RTL

[logonoff]

Refactor all test files to use renderHook from @testing-library/react instead of the deprecated custom testHook utility. This change:

• Updates 28 test files across multiple packages to use renderHook
• Removes the deprecated hooks-utils.tsx file containing testHook
• Fixes tests that had assertions inside hook callbacks to properly use result.current
• Removes callback-based patterns (done()) in favor of synchronous assertions

Summary by CodeRabbit

• Tests
  • Modernized testing infrastructure by replacing deprecated custom hook testing utility with standard React Testing Library's renderHook across multiple test suites throughout the codebase. Removed legacy test helper. No changes to application functionality or user experience.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[openshift-ci-robot]

@logonoff: This pull request references CONSOLE-5041 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set.

Details

In response to this:

Refactor all test files to use renderHook from @testing-library/react instead of the deprecated custom testHook utility. This change:

• Updates 28 test files across multiple packages to use renderHook
• Removes the deprecated hooks-utils.tsx file containing testHook
• Fixes tests that had assertions inside hook callbacks to properly use result.current
• Removes callback-based patterns (done()) in favor of synchronous assertions

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: logonoff

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• frontend/OWNERS [logonoff]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[logonoff]

/label px-approved
/label docs-approved
/assign @cajieh @vojtechszocs

changes only affect unit tests:
/verified by CI

[openshift-ci-robot]

@logonoff: This PR has been marked as verified by CI.

Details

In response to this:

/label px-approved
/label docs-approved
/assign @cajieh @vojtechszocs

changes only affect unit tests:
/verified by CI

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci-robot]

@logonoff: This pull request references CONSOLE-5041 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set.

Details

In response to this:

Refactor all test files to use renderHook from @testing-library/react instead of the deprecated custom testHook utility. This change:

• Updates 28 test files across multiple packages to use renderHook
• Removes the deprecated hooks-utils.tsx file containing testHook
• Fixes tests that had assertions inside hook callbacks to properly use result.current
• Removes callback-based patterns (done()) in favor of synchronous assertions

Summary by CodeRabbit

• Tests
• Modernized testing infrastructure by replacing deprecated custom hook testing utility with standard React Testing Library's renderHook across multiple test suites throughout the codebase. Removed legacy test helper. No changes to application functionality or user experience.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request systematically replaces a deprecated custom testing utility (testHook) with React Testing Library's standard renderHook function across multiple test files throughout the codebase. The testHook utility and its supporting types are removed from the shared test-utils module. All test files are updated to import renderHook from @testing-library/react and adjusted to use the standard React Testing Library API for hook testing. Test logic, assertions, and expected outcomes remain unchanged; only the testing mechanism and invocation pattern are updated.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically identifies the main refactoring: migrating from a deprecated custom testHook utility to the standard renderHook from React Testing Library.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (6)

frontend/packages/dev-console/src/utils/__tests__/useAddActionExtensions.spec.ts (3)

75-79: Move assertions outside the renderHook callback.

The PR description explicitly mentions "Fixes tests that had assertions inside hook callbacks to properly use result.current," but these tests still have assertions inside the callback. This is an anti-pattern—the callback should only render the hook, while assertions should operate on result.current.

Suggested fix

-    renderHook(() => {
-      const [addActionExtensions, resolved] = useAddActionExtensions();
-      expect(addActionExtensions).toEqual([addAction1, addAction2, addAction3, addAction4]);
-      expect(resolved).toEqual(true);
-    });
+    const { result } = renderHook(() => useAddActionExtensions());
+    const [addActionExtensions, resolved] = result.current;
+    expect(addActionExtensions).toEqual([addAction1, addAction2, addAction3, addAction4]);
+    expect(resolved).toEqual(true);

---

89-93: Same pattern issue—assertions belong outside the callback.

Suggested fix

-    renderHook(() => {
-      const [addActionExtensions, resolved] = useAddActionExtensions();
-      expect(addActionExtensions).toEqual([addAction1, addAction2, addAction3, addAction4]);
-      expect(resolved).toEqual(true);
-    });
+    const { result } = renderHook(() => useAddActionExtensions());
+    const [addActionExtensions, resolved] = result.current;
+    expect(addActionExtensions).toEqual([addAction1, addAction2, addAction3, addAction4]);
+    expect(resolved).toEqual(true);

---

103-107: Same pattern issue—assertions belong outside the callback.

Suggested fix

-    renderHook(() => {
-      const [addActionExtensions, resolved] = useAddActionExtensions();
-      expect(addActionExtensions).toEqual([addAction1, addAction3, addAction4]);
-      expect(resolved).toEqual(true);
-    });
+    const { result } = renderHook(() => useAddActionExtensions());
+    const [addActionExtensions, resolved] = result.current;
+    expect(addActionExtensions).toEqual([addAction1, addAction3, addAction4]);
+    expect(resolved).toEqual(true);

frontend/packages/dev-console/src/utils/__tests__/usePerspectiveDetection.spec.ts (2)

16-21: Assertions should be outside the renderHook callback.

Same issue as the other files—the expect calls should operate on result.current after renderHook returns, not inside the callback function.

Suggested fix

-    renderHook(() => {
-      const [enablePerspective, loading] = usePerspectiveDetection();
-
-      expect(enablePerspective).toBe(true);
-      expect(loading).toBe(true);
-    });
+    const { result } = renderHook(() => usePerspectiveDetection());
+    const [enablePerspective, loading] = result.current;
+
+    expect(enablePerspective).toBe(true);
+    expect(loading).toBe(true);

---

29-34: Same pattern issue—move assertions outside.

Suggested fix

-    renderHook(() => {
-      const [enablePerspective, loading] = usePerspectiveDetection();
-
-      expect(enablePerspective).toBe(true);
-      expect(loading).toBe(false);
-    });
+    const { result } = renderHook(() => usePerspectiveDetection());
+    const [enablePerspective, loading] = result.current;
+
+    expect(enablePerspective).toBe(true);
+    expect(loading).toBe(false);

frontend/packages/webterminal-plugin/src/components/cloud-shell/__tests__/useActivityTick.spec.ts (1)

24-54: Refactor to use result.current for the tick function.

This test has significant logic inside the renderHook callback. While the time-manipulation testing approach is valid, the assertions and tick() calls should happen outside the callback using result.current.

Suggested refactor

-    renderHook(() => {
-      const tick = useActivityTick('testName', 'testNamespace');
-
-      // send initial tick
-      tick();
-      expect(sendActivityTickMock).toHaveBeenCalledWith('testName', 'testNamespace');
-      sendActivityTickMock.mockReset();
-
-      // elapsed time isn't long enough to trigger sending a new tick
-      mockNow += 59000;
-      tick();
-
-      // reset mock to test count
-      expect(sendActivityTickMock).toHaveBeenCalledTimes(0);
-
-      // elapsed time is now long enough to trigger sending a tick
-      mockNow += 1000;
-      tick();
-      expect(sendActivityTickMock).toHaveBeenCalledWith('testName', 'testNamespace');
-      expect(sendActivityTickMock).toHaveBeenCalledTimes(1);
-
-      // multiple ticks within the time interval does not result sending tick
-      tick();
-      tick();
-      expect(sendActivityTickMock).toHaveBeenCalledTimes(1);
-
-      // advance time enough to send a tick
-      mockNow += 600000;
-      tick();
-      expect(sendActivityTickMock).toHaveBeenCalledTimes(2);
-    });
+    const { result } = renderHook(() => useActivityTick('testName', 'testNamespace'));
+    const tick = result.current;
+
+    // send initial tick
+    tick();
+    expect(sendActivityTickMock).toHaveBeenCalledWith('testName', 'testNamespace');
+    sendActivityTickMock.mockReset();
+
+    // elapsed time isn't long enough to trigger sending a new tick
+    mockNow += 59000;
+    tick();
+
+    // reset mock to test count
+    expect(sendActivityTickMock).toHaveBeenCalledTimes(0);
+
+    // elapsed time is now long enough to trigger sending a tick
+    mockNow += 1000;
+    tick();
+    expect(sendActivityTickMock).toHaveBeenCalledWith('testName', 'testNamespace');
+    expect(sendActivityTickMock).toHaveBeenCalledTimes(1);
+
+    // multiple ticks within the time interval does not result sending tick
+    tick();
+    tick();
+    expect(sendActivityTickMock).toHaveBeenCalledTimes(1);
+
+    // advance time enough to send a tick
+    mockNow += 600000;
+    tick();
+    expect(sendActivityTickMock).toHaveBeenCalledTimes(2);

🧹 Nitpick comments (6)

frontend/packages/webterminal-plugin/src/components/cloud-shell/__tests__/useCloudShellAvailable.spec.ts (1)

32-51: Tests will pass, but the rerender() call is unnecessary for these async assertions.

The initial renderHook() already triggers useEffect, which calls checkTerminalAvailable(). The await act(async () => {...}) wrapper flushes the microtask queue, allowing the mocked promise to settle. The rerender() inside doesn't contribute to waiting for the async operation.

A more idiomatic RTL pattern:

const { result } = renderHook(() => useCloudShellAvailable());
await act(async () => {
  // Let microtasks flush (promise resolution/rejection)
});
expect(result.current).toBe(false);

Or using waitFor for explicit state change detection:

import { renderHook, waitFor } from '@testing-library/react';
// ...
const { result } = renderHook(() => useCloudShellAvailable());
await waitFor(() => {
  expect(result.current).toBe(true);
});

Given this is a bulk migration PR, the current approach is functional and maintains consistency across files. Consider revisiting as a follow-up if you want tighter alignment with RTL best practices.

frontend/packages/console-shared/src/utils/__tests__/pod-ring-utils.spec.ts (1)

214-228: Async API call tests may not await completion.

These tests exercise code paths where checkPodEditAccess is invoked asynchronously (per the hook implementation in pod-ring-utils.ts). The assertions fire immediately after renderHook without using waitFor or act. They pass because the initial state (false) happens to match the expected final state, but they don't actually verify that the async operation completed.

Consider using waitFor to ensure the hook's effect completes:

♻️ Suggested pattern using waitFor

+import { renderHook, waitFor } from '@testing-library/react';
 ...
-  it('should return false when api call returns false for a resource', () => {
+  it('should return false when api call returns false for a resource', async () => {
     obj.kind = 'DeploymentConfig';
     const { result } = renderHook(() =>
       usePodScalingAccessStatus(obj, DeploymentConfigModel, [], true),
     );
+    await waitFor(() => {
+      expect(checkPodEditAccessMock).toHaveBeenCalled();
+    });
     expect(result.current).toBe(false);
   });

This ensures the mock was actually invoked and the async path was exercised, improving test reliability.

frontend/packages/topology/src/filters/__tests__/useSearchFilter.spec.ts (2)

34-41: Assertions inside renderHook callback is an anti-pattern.

The assertions are placed inside the renderHook callback rather than using result.current outside. While this works because the helper testUseSearchFilter invokes the hook synchronously, this pattern couples test assertions to the render phase and deviates from RTL's intended usage.

The idiomatic pattern would restructure tests to invoke testUseSearchFilter via renderHook and then assert on result.current:

♻️ Suggested refactor for idiomatic usage

 it('should handle null | undefined text', () => {
-  renderHook(() => {
-    expect(testUseSearchFilter(null, 'test')[0]).toBe(false);
-    expect(testUseSearchFilter(null, '')[0]).toBe(false);
-    expect(testUseSearchFilter(null, undefined)[0]).toBe(false);
-    expect(testUseSearchFilter(undefined, 'test')[0]).toBe(false);
-    expect(testUseSearchFilter(undefined, '')[0]).toBe(false);
-    expect(testUseSearchFilter(undefined, undefined)[0]).toBe(false);
-  });
+  expect(renderHook(() => testUseSearchFilter(null, 'test')).result.current[0]).toBe(false);
+  expect(renderHook(() => testUseSearchFilter(null, '')).result.current[0]).toBe(false);
+  expect(renderHook(() => testUseSearchFilter(null, undefined)).result.current[0]).toBe(false);
+  expect(renderHook(() => testUseSearchFilter(undefined, 'test')).result.current[0]).toBe(false);
+  expect(renderHook(() => testUseSearchFilter(undefined, '')).result.current[0]).toBe(false);
+  expect(renderHook(() => testUseSearchFilter(undefined, undefined)).result.current[0]).toBe(false);
 });

Alternatively, consider refactoring testUseSearchFilter to just set up mocks, and have each test call renderHook(() => useSearchFilter(...)) directly for cleaner separation.

---

44-92: Same pattern issue applies to remaining test cases.

All subsequent test cases (lines 44-92) follow the same anti-pattern of placing assertions inside the renderHook callback. Consider applying the same refactoring approach consistently across all tests in this file for maintainability.

frontend/packages/console-shared/src/hooks/__tests__/usePinnedResources.spec.ts (1)

63-83: Clean migration — consider removing vestigial async keywords.

The renderHook migration is correctly implemented. However, the test functions are marked async but don't contain any await statements. This is likely a remnant from the previous testHook usage. While harmless, removing the async keywords would improve clarity.

♻️ Optional: Remove unnecessary async

-it('should return default pins from extension if perspectives are not configured', async () => {
+it('should return default pins from extension if perspectives are not configured', () => {

Apply similarly to all test cases in this file.

frontend/packages/dev-console/src/components/add/hooks/__tests__/useAccessFilterExtensions.spec.ts (1)

27-35: Return hook result and assert via result.current

Inline assertions inside the render callback can miss updates and make timing brittle. Return the hook result and assert outside (use waitFor if updates are async).

♻️ Proposed refactor

-    renderHook(() => {
-      useAddActionsAccessReviewsSpy.mockReturnValue(mockAddAccessReviewResults);
-      const [filteredAddActionExtensions, loaded] = useAccessFilterExtensions(
-        namespace,
-        addActionExtensions,
-      );
-      expect(filteredAddActionExtensions.length).toEqual(0);
-      expect(loaded).toBe(false);
-    });
+    useAddActionsAccessReviewsSpy.mockReturnValue(mockAddAccessReviewResults);
+    const { result } = renderHook(() =>
+      useAccessFilterExtensions(namespace, addActionExtensions),
+    );
+    const [filteredAddActionExtensions, loaded] = result.current;
+    expect(filteredAddActionExtensions.length).toEqual(0);
+    expect(loaded).toBe(false);

-    renderHook(() => {
-      useAddActionsAccessReviewsSpy.mockReturnValue(mockAccessReviewResults);
-      const [filteredAddActionExtensions, loaded] = useAccessFilterExtensions(
-        namespace,
-        addActionExtensions,
-      );
-      expect(filteredAddActionExtensions.length).toEqual(accessAllowedResults.length);
-      expect(loaded).toBe(true);
-    });
+    useAddActionsAccessReviewsSpy.mockReturnValue(mockAccessReviewResults);
+    const { result } = renderHook(() =>
+      useAccessFilterExtensions(namespace, addActionExtensions),
+    );
+    const [filteredAddActionExtensions, loaded] = result.current;
+    expect(filteredAddActionExtensions.length).toEqual(accessAllowedResults.length);
+    expect(loaded).toBe(true);

Also applies to: 48-56

[logonoff]

changes only affect unit tests:
/verified by CI

[openshift-ci-robot]

@logonoff: This PR has been marked as verified by CI.

Details

In response to this:

changes only affect unit tests:
/verified by CI

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[cajieh]

@logonoff The migration from testHook to RTL renderHook looks good! 👍
However, a few files still have assertions inside the renderHook callback. Added comments. Thank you.

[logonoff]

/verified by CI

[openshift-ci-robot]

@logonoff: This PR has been marked as verified by CI.

Details

In response to this:

/verified by CI

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[logonoff]

/verified by CI

[openshift-ci-robot]

@logonoff: This PR has been marked as verified by CI.

Details

In response to this:

/verified by CI

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci]

@logonoff: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.