-
Notifications
You must be signed in to change notification settings - Fork 361
chore: Use local clickhouse instance for playwright tests #1711
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Code Review✅ No critical issues found. Minor observations:
Strengths:
The PR successfully replaces dependency on external demo server with local ClickHouse, improving test reliability and control. |
- Add Docker Compose service for local ClickHouse instance - Add init script to create e2e_* tables in ClickHouse - Add seed-clickhouse.ts to populate test data - Add global-setup-local.ts for local mode setup - Update test scripts to support local ClickHouse - Update .env.e2e to point to local ClickHouse instead of demo server - Add stop-e2e.sh script for cleanup This enables E2E tests to run against a local Docker ClickHouse instance with seeded test data, instead of relying on the public demo server. Both full-stack mode (MongoDB + API + ClickHouse) and local mode (frontend + ClickHouse) now use the same local ClickHouse instance.
The global-setup-fullstack.ts was missing the seedClickHouse() call, which meant tests had no data to work with.
- Add saveSearchAndWaitForNavigation() to avoid race condition - Starts waiting for URL change BEFORE submitting form - Eliminates flaky tests where modal closes but URL hasn't changed yet - Replaces manual pattern of await modal hidden + await URL change - Simplify search-filters.spec.ts to use known test data - Use hardcoded 'info' severity from seeded data instead of dynamic discovery - Remove 18 lines of complex conditional logic - More reliable and easier to understand These changes leverage consistent test data from ClickHouse seeding.
E2E Test Results❌ 1 test failed • 65 passed • 4 skipped • 791s
Tests ran across 4 shards in parallel. |
3d7cb2b to
8d16aca
Compare
| await test.step('Navigate between each page', async () => { | ||
| for (const { testId, contentTestId } of navLinks) { | ||
| const link = page.locator(`[data-testid="${testId}"]`); | ||
| await link.scrollIntoViewIfNeeded(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reduces flakiness
| env.DEFAULT_CONNECTIONS = JSON.stringify(fixture.connections ?? []); | ||
| env.DEFAULT_SOURCES = JSON.stringify(fixture.sources ?? []); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simplify loading connections/sources instead of one big blob in an env file
| // Optionally wait for modal to fully close | ||
| await expect(this.container).toBeHidden(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
abstraction for saved searches (lots of flakiness exposed when moving away from play demo server)
|
|
||
| await expect(searchPage.savedSearchModal.container).toBeHidden(); | ||
| await page.waitForURL(/\/search\/[a-f0-9]+/, { timeout: 5000 }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simplify these tests with more predictable input
| run: | ||
| docker compose -p e2e -f packages/app/tests/e2e/docker-compose.yml | ||
| down -v | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use docker file instead
| ORDER BY (ServiceName, MetricName, Attributes, toUnixTimestamp64Nano(TimeUnix)) | ||
| TTL toDate(TimeUnix) + toIntervalDay(30) | ||
| SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1 | ||
| EOFSQL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create schema for local db
|
|
||
| # DEFAULT_CONNECTIONS and DEFAULT_SOURCES are injected from packages/app/tests/e2e/fixtures/e2e-fixtures.json | ||
| # by the e2e API runner (run-api-with-fixtures.js) and by base-test for local mode. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to fixtures
| //verify there is a pin icon | ||
| const pinIcon = searchPage.page.getByTestId( | ||
| `filter-pin-${availableFilterValue!}-pinned`, | ||
| `filter-pin-${TEST_FILTER_VALUE}-pinned`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
greatly simplify this logic now that we completely control the data
| @@ -1,3 +1,5 @@ | |||
| // We may want to add logs/metrics source names here in the future. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
| // We may want to add logs/metrics source names here in the future. |
scripts/stop-e2e.sh
Outdated
| @@ -0,0 +1,10 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file used anywhere?
| @@ -0,0 +1,209 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to run chmod +x docker/clickhouse/local/init-db-e2e.sh to get this script to run in docker desktop - are you able to run the same and commit the extra permission to this PR?
Makefile
Outdated
| if [ -n "$(tags)" ]; then set -- "$$@" --tags "$(tags)"; fi; \ | ||
| if [ "$(ui)" = "true" ]; then set -- "$$@" --ui; fi; \ | ||
| ./scripts/test-e2e.sh "$$@" | ||
| @# For more control (--ui, --last-failed, --headed, etc), call the script directly: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packages/app/tests/e2e/README.md still suggests make e2e ui=true, could that be updated?
| const rows: string[] = []; | ||
|
|
||
| for (let i = 0; i < count; i++) { | ||
| const timestampNs = (now - i * 60000) * 1000000; // 1 minute intervals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I noticed while running the tests locally for a few minutes is that tests (the search tests for example) quickly start failing because data is not continuously being inserted for newer/more current timestamps, so when a search filters for "last 5 minutes" there is no data, if seeding ran >5 minutes ago. Would it make sense to see data "in the future" as well, to prevent this sort of thing in most cases?
a19cf47 to
962daf9
Compare
…dx into tom/e2e-local-ch-minimal
TLDR: This PR changes playwright full-stack tests to run against a local clickhouse instance (with seeded data) instead of relying on the clickhouse demo server, which can be unpredictable at times. This workflow allows us to fully control the data to make tests more predictable.
This PR:
Fixes: HDX-3193