test: Add testing infra for the presto-ui#27144
Merged
yhwang merged 1 commit intoprestodb:masterfrom Mar 6, 2026
Merged
Conversation
prestodb-ci
requested review from
a team,
Mariamalmesfer and
sh-shamsan
and removed request for
a team
Contributor
Reviewer's GuideAdds a complete Jest + React Testing Library test infrastructure for presto-ui (including fixtures, mocks, helpers, config, and scripts), wires it into ESLint/TypeScript/CI, and fixes a bug in QueryList search state handling. Sequence diagram for the updated check_webui.sh pipeline with Jest testssequenceDiagram
actor Developer
participant CI as CI_runner
participant Script as check_webui.sh
participant Yarn as yarn
participant TS as TypeScript_checker
participant ESL as ESLint
participant Jest as Jest_test_runner
Developer->>CI: Push changes / open PR
CI->>Script: Execute check_webui.sh
Script->>Yarn: yarn --cwd presto-ui/src run build
Yarn-->>Script: Build result
alt Build failed
Script-->>CI: exit 1 (build error)
CI-->>Developer: Report build failure
else Build succeeded
Script->>TS: Run TypeScript type checks
TS-->>Script: Type check result
alt TypeScript errors
Script-->>CI: exit 1 (type check error)
CI-->>Developer: Report TypeScript errors
else TypeScript clean
Script->>ESL: yarn --cwd presto-ui/src run lint --quiet
ESL-->>Script: Lint result
alt ESLint errors
Script-->>CI: exit 1 (ESLint errors)
CI-->>Developer: Report lint errors
else Lint clean
Script->>Jest: yarn --cwd presto-ui/src run test:ci
Jest-->>Script: Test result (--ci --coverage --maxWorkers=2)
alt Tests failed
Script-->>CI: exit 1 (tests failed)
CI-->>Developer: Report Jest test failures
else Tests passed
Script-->>CI: exit 0 (all checks passed)
CI-->>Developer: Report success
end
end
end
end
Class diagram for Jest configuration and global test setup modulesclassDiagram
class JestConfig {
+string preset
+string testEnvironment
+string[] roots
+string[] testMatch
+map~string,string~ transform
+map~string,string~ moduleNameMapper
+string[] transformIgnorePatterns
+string[] setupFilesAfterEnv
+string[] collectCoverageFrom
+string[] coverageReporters
+string[] testPathIgnorePatterns
+string[] moduleFileExtensions
}
class SetupTests {
+void setupAllBrowserMocks()
+void installJQueryMock()
+void patchConsoleWarn()
}
class BrowserMocks {
+void setupAllBrowserMocks()
}
class JQueryMock {
+any createJQueryMock()
}
class PageTitleTest {
}
class QueryListTests {
}
class UtilsTests {
}
JestConfig --> SetupTests : setupFilesAfterEnv
SetupTests ..> BrowserMocks : uses
SetupTests ..> JQueryMock : uses
PageTitleTest ..> SetupTests : runs after setup
QueryListTests ..> SetupTests : runs after setup
UtilsTests ..> SetupTests : runs after setup
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The fetch mocking is split between
browserMocks.tsandapiMocks.ts(both definingsetupFetchMockand also invoked globally insetupTests.ts), which makes it harder to reason about which implementation is active; consider consolidating to a single, clearly-named fetch mock entry point to avoid accidental overrides and confusion. - The global
console.warnoverride insetupTests.tsfilters by substring and may accidentally hide unrelated warnings in the future; it would be safer to scope this suppression to specific tests or to match more narrowly (e.g., by checking the full message and Jest stack) so that new legitimate warnings still surface.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The fetch mocking is split between `browserMocks.ts` and `apiMocks.ts` (both defining `setupFetchMock` and also invoked globally in `setupTests.ts`), which makes it harder to reason about which implementation is active; consider consolidating to a single, clearly-named fetch mock entry point to avoid accidental overrides and confusion.
- The global `console.warn` override in `setupTests.ts` filters by substring and may accidentally hide unrelated warnings in the future; it would be safer to scope this suppression to specific tests or to match more narrowly (e.g., by checking the full message and Jest stack) so that new legitimate warnings still surface.
## Individual Comments
### Comment 1
<location> `presto-ui/src/jest.config.js:15-27` </location>
<code_context>
+ },
+ transformIgnorePatterns: ["node_modules/(?!(dagre-d3-es|d3|d3-.*|internmap|delaunator|robust-predicates)/)"],
+ setupFilesAfterEnv: ["<rootDir>/setupTests.ts"],
+ collectCoverageFrom: [
+ "**/*.{js,jsx,ts,tsx}",
+ "!**/*.d.ts",
+ "!sql-parser/**",
+ "!static/**",
+ "!templates/**",
+ "!webpack.config.js",
+ "!**/*.test.{js,jsx,ts,tsx}",
+ "!**/*.spec.{js,jsx,ts,tsx}",
+ "!jest.config.js",
+ "!setupTests.ts",
+ "!__tests__/**",
+ ],
+ // Coverage thresholds disabled - focus on test pass rate
</code_context>
<issue_to_address>
**suggestion (testing):** Exclude the coverage output directory from collectCoverageFrom to avoid scanning generated files.
Because `collectCoverageFrom` uses a broad pattern (`"**/*.{js,jsx,ts,tsx}"`) and Jest’s default `coverageDirectory` is `"coverage"`, any JS/TS that ends up in `coverage/` could be re-scanned. Add an exclusion such as `"!coverage/**"` or `"!<rootDir>/coverage/**"` to the list to prevent that.
```suggestion
collectCoverageFrom: [
"**/*.{js,jsx,ts,tsx}",
"!**/*.d.ts",
"!sql-parser/**",
"!static/**",
"!templates/**",
"!webpack.config.js",
"!**/*.test.{js,jsx,ts,tsx}",
"!**/*.spec.{js,jsx,ts,tsx}",
"!jest.config.js",
"!setupTests.ts",
"!__tests__/**",
"!<rootDir>/coverage/**",
],
```
</issue_to_address>
### Comment 2
<location> `presto-ui/src/setupTests.ts:35-44` </location>
<code_context>
+// Suppress harmless fake timer warnings
+// These occur when async helpers call jest.advanceTimersByTime()
+// but fake timers ARE properly set up in beforeEach
+const originalWarn = console.warn;
+console.warn = (...args: any[]) => {
+ const message = args[0]?.toString() || "";
+
+ // Suppress fake timer warnings - they're harmless
+ if (message.includes("A function to advance timers was called but the timers APIs are not replaced")) {
+ return;
+ }
+
+ // Allow all other warnings
+ originalWarn.apply(console, args);
+};
+```
</code_context>
<issue_to_address>
**issue (testing):** Global console.warn override may interfere with tests that assert on warnings or spy on console.
This global override forces any test that spies on or asserts against `console.warn` to deal with the wrapper and may break code that depends on the original implementation. To narrow the impact, consider: (1) matching the full Jest timer warning string exactly and only when `process.env.JEST_WORKER_ID` is set, or (2) handling this in the specific suites that trigger the warning via a scoped `jest.spyOn(console, "warn")` filter instead of a global override.
</issue_to_address>
### Comment 3
<location> `presto-ui/src/__tests__/utils/testUtils.tsx:191-194` </location>
<code_context>
+ * await typeIntoInput(/search/i, 'test');
+ * await advanceTimersAndWait(300); // Wait for debounce
+ */
+export const advanceTimersAndWait = async (ms: number) => {
+ jest.advanceTimersByTime(ms);
+ await waitFor(() => {}, { timeout: 0 });
</code_context>
<issue_to_address>
**suggestion:** `advanceTimersAndWait` assumes fake timers are active but doesn’t enforce or assert it
Since this calls `jest.advanceTimersByTime`, tests that forget to enable fake timers will fail with a confusing error. Consider either enabling/cleaning up fake timers inside this helper, or adding a guard that throws a clear message when fake timers aren’t active (e.g., when used with `setupFakeTimers` / `setupIntegrationTest`).
```suggestion
export const advanceTimersAndWait = async (ms: number) => {
try {
jest.advanceTimersByTime(ms);
} catch (error) {
// Provide a clearer error when fake timers are not enabled
const originalMessage = error instanceof Error ? error.message : String(error);
throw new Error(
[
'advanceTimersAndWait requires Jest fake timers to be enabled.',
'Make sure to call setupFakeTimers() or setupIntegrationTest() before using this helper.',
`Original Jest error: ${originalMessage}`,
].join(' ')
);
}
await waitFor(() => {}, { timeout: 0 });
};
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
yhwang
force-pushed
the
presto-ui-testing
branch
3 times, most recently
from
27dad0d to
1cae54c
Compare
Member
Author
|
Addressed sourcery-ai's comments. For |
Member
Author
|
@johallar and @cmatzenbach could you review this PR, since you contributed to the Presto UI? The goal is to enable the testing first, add the testing infrastructure, and the needed libraries. Then I will gradually add test cases for other components. It would be great to have end-to-end tests to verify the REST APIs with the UI components. |
johallar
reviewed
Feb 24, 2026
johallar
reviewed
Feb 24, 2026
johallar
reviewed
Feb 24, 2026
johallar
reviewed
Feb 24, 2026
steveburnett
previously approved these changes
Feb 24, 2026
Contributor
steveburnett
left a comment
steveburnett
left a comment
There was a problem hiding this comment.
LGTM! (docs)
View README.md and it looks very good. Thanks!
yhwang
force-pushed
the
presto-ui-testing
branch
from
1cae54c to
aa146c8
Compare
Member
Author
|
@johallar, appreciate your review. I believe I addressed all your comments. Let me know if any changes are needed. |
johallar
reviewed
Feb 26, 2026
johallar
reviewed
Feb 26, 2026
johallar
reviewed
Feb 26, 2026
johallar
reviewed
Feb 26, 2026
johallar
approved these changes
Feb 26, 2026
Contributor
johallar
left a comment
johallar
left a comment
There was a problem hiding this comment.
Looks great! Left a couple final thoughts, take them or leave them
yhwang
force-pushed
the
presto-ui-testing
branch
from
aa146c8 to
06c2dca
Compare
Member
Author
|
@johallar Thanks again for those great suggestions. As I mentioned above, let's keep the initial code base clean. We can add them back when we need them. |
Add Jest 29.7.0 with React Testing Library 16.1.0 to enable comprehensive component and integration testing for the Presto UI dashboard. BREAKING CHANGE: None Features: - Jest 29.7.0 with ts-jest and babel-jest configuration - React Testing Library 16.1.0 for component testing - @testing-library/user-event 14.5.2 for user interactions - Test fixtures with factory pattern (query, info, stage, cluster) - Comprehensive mocks (jQuery, Fetch API, Browser APIs) - 10+ test utility helpers in testUtils.tsx - 8 component-specific setup patterns in setupHelpers.ts - Barrel exports for fixtures and mocks Bug Fixes: - Fix a bug in QueryList.jsx:342 where getHumanReadableState() was called with incorrect parameters Started with 4 ts files: utils.ts, d3utils.ts, components/PageTitle.tsx, and router/QueryList.tsx. Signed-off-by: Yihong Wang <yh.wang@ibm.com>
yhwang
force-pushed
the
presto-ui-testing
branch
from
06c2dca to
a88d720
Compare
Member
Author
|
Just updated the commit message to rerun the CI jobs. No code change. There were some stale jobs earlier. |
Member
Author
|
Hi @tdcmeehan, I incorporated Joe's great suggestions/comments and believe it's in good shape now. How do you think about the changes? Or anyone you'd like to invite to the review? |
Contributor
|
@yhwang is there any Github action that runs the tests? If not, let's add it now! |
Member
Author
Good question. I've updated presto-ui/bin/check_webui.sh to run the tests in the GH action. One more thing we can further do is: once we add test cases to all components and reach a certain code coverage, we can also set a threshold to enforce it and fail the GH action if the coverage is low. |
Member
Author
|
If there is no further review comment, should we move forward and enable the testing framework for the Presto UI? |
Contributor
|
@yhwang got it, the existing |
tdcmeehan
approved these changes
Mar 6, 2026
garimauttam
pushed a commit
to garimauttam/presto
that referenced
this pull request
Mar 9, 2026
## Description Add Jest 29.7.0 with React Testing Library 16.1.0 to enable comprehensive component and integration testing for the Presto UI dashboard. BREAKING CHANGE: None Features: - Jest 29.7.0 with ts-jest and babel-jest configuration - React Testing Library 16.1.0 for component testing - @testing-library/user-event 14.5.2 for user interactions - Test fixtures with factory pattern (query, info, stage, cluster) - Comprehensive mocks (jQuery, Fetch API, Browser APIs) - 22+ test utility helpers in testUtils.tsx - 8 component-specific setup patterns in setupHelpers.ts - Barrel exports for fixtures and mocks Bug Fixes: - Fix a bug in QueryList.jsx:342 where getHumanReadableState() was called with incorrect parameters Started with 4 ts files: utils.ts, d3utils.ts, components/PageTitle.tsx, and router/QueryList.tsx. ## Motivation and Context There are no tests for the presto-ui. ## Impact Start to add test cases for the presto-ui and improve the code coverage. ## Test Plan The CI will run the presto-ui test cases. ## Contributor checklist - [x] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards). - [x] PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced. - [ ] Documented new properties (with its default value), SQL syntax, functions, or other functionality. - [ ] If release notes are required, they follow the [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [ ] Adequate tests were added if applicable. - [ ] CI passed. - [ ] If adding new dependencies, verified they have an [OpenSSF Scorecard](https://securityscorecards.dev/#the-checks) score of 5.0 or higher (or obtained explicit TSC approval for lower scores). ## Release Notes Please follow [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines) and fill in the release notes below. ``` == NO RELEASE NOTE == ``` ## Summary by Sourcery Introduce a Jest-based React testing infrastructure for the Presto UI and fix query state filtering in the QueryList view. New Features: - Add Jest and React Testing Library configuration, including Babel and TypeScript integration, for running UI tests in presto-ui. - Provide shared test utilities, setup helpers, mocks, and fixtures to simplify writing component and integration tests for the dashboard. Bug Fixes: - Correct QueryList search filtering to call getHumanReadableState with the full query state payload, ensuring accurate state-based search. Enhancements: - Update ESLint and TypeScript configurations to recognize Jest globals in test files and exclude test artifacts from linting and coverage. - Extend the presto-ui check script to run Jest tests in CI alongside TypeScript and ESLint checks. Build: - Add Jest, ts-jest, babel-jest, RTL, and related dev dependencies plus Babel config and npm scripts for running tests and collecting coverage in presto-ui. Documentation: - Add a Presto UI Testing Guide documenting test structure, utilities, mocks, fixtures, and recommended testing patterns. - Document test-running commands and patterns in the new testing README under __tests__. Tests: - Add initial unit and integration test files for core UI areas including utils, d3utils, PageTitle, QueryList, and QueryListItem. Signed-off-by: Yihong Wang <yh.wang@ibm.com>
This was referenced Mar 31, 2026
15 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add Jest 29.7.0 with React Testing Library 16.1.0 to enable comprehensive component and integration testing for the Presto UI dashboard.
BREAKING CHANGE: None
Features:
Bug Fixes:
Started with 4 ts files: utils.ts, d3utils.ts, components/PageTitle.tsx, and router/QueryList.tsx.
Motivation and Context
There are no tests for the presto-ui.
Impact
Start to add test cases for the presto-ui and improve the code coverage.
Test Plan
The CI will run the presto-ui test cases.
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
Summary by Sourcery
Introduce a Jest-based React testing infrastructure for the Presto UI and fix query state filtering in the QueryList view.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests: