perf(ci): use playwright ingestion_dependency in postgresql e2e#26893
perf(ci): use playwright ingestion_dependency in postgresql e2e#26893chirag-madlani wants to merge 4 commits intomainfrom
Conversation
Replaces `ingestion_dependency: "all"` with `"playwright"` which installs only the connectors needed for E2E tests (mysql, bigquery, kafka, mlflow, snowflake). Runs across all 6 shards so the savings compound. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions
bot
added
safe to test
- Cache Maven dependencies in shard jobs (restore from build job's cache) - Cache Python venv keyed on setup.py/pyproject.toml hashes to skip repeated ingestion[playwright] installs across shards and reruns - Cache Node modules (node_modules) keyed on yarn.lock; skip yarn install on cache hit - Cache Playwright browser binaries keyed on playwright version; skip install on cache hit - Bump CI workers from 3 to 4 to utilise all vCPUs on ubuntu-latest Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Optimizes the PostgreSQL Playwright E2E CI workflow by changing the ingestion container dependency set from the full all extra to the narrower playwright extra, aiming to reduce CI runtime/resources for this workflow.
Changes:
- Update
ingestion_dependencypassed to the test-environment setup action from"all"to"playwright".
| python-version: "3.10" | ||
| args: "-d postgresql -s true" | ||
| ingestion_dependency: "all" | ||
| ingestion_dependency: "playwright" | ||
|
|
There was a problem hiding this comment.
The ingestion_dependency input here only affects the Docker build arg (INGESTION_DEPENDENCY) used by docker/run_local_docker.sh (i.e., what gets installed inside the ingestion container). The composite action setup-openmetadata-test-environment still installs ingestion[all] + ingestion[test] in the runner venv unconditionally, so this change likely won’t deliver the “only install playwright-specific dependencies” CI-time reduction described unless that action is also updated (or the PR description clarifies the scope of the optimization).
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| uses: actions/cache@v4 | ||
| with: | ||
| path: ~/.cache/ms-playwright | ||
| key: ${{ runner.os }}-playwright-1.57.0 |
There was a problem hiding this comment.
💡 Edge Case: Hardcoded Playwright version in cache key may drift from package.json
The Playwright browser cache key is hardcoded to playwright-1.57.0 (line 177), while the install command also hardcodes npx playwright@1.57.0. If @playwright/test is bumped in package.json, these hardcoded references will silently become stale, causing version mismatches between the test runner and cached browsers. Other workflows (e.g., playwright-mysql-e2e.yml) have the same pattern, so this is a pre-existing issue, but this PR is adding a new instance of it.
Suggested fix:
Consider deriving the version dynamically, e.g.:
key: ${{ runner.os }}-playwright-${{ hashFiles('openmetadata-ui/src/main/resources/ui/yarn.lock') }}
Or extract the version into a workflow-level env var to reduce duplication.
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
The ingestion_dependency input was only passed to run_local_docker.sh (Docker build arg) but the runner venv still installed ingestion[all] unconditionally. Now both the Docker container and the runner venv respect the same ingestion_dependency value. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| - name: Cache Python venv | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: env | ||
| key: ${{ runner.os }}-python-3.10-ingestion-playwright-${{ hashFiles('ingestion/setup.py', 'ingestion/pyproject.toml') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-python-3.10-ingestion-playwright- | ||
|
|
||
| - name: Setup Openmetadata Test Environment | ||
| uses: ./.github/actions/setup-openmetadata-test-environment | ||
| with: | ||
| python-version: "3.10" | ||
| args: "-d postgresql -s true" | ||
| ingestion_dependency: "all" | ||
| ingestion_dependency: "playwright" |
There was a problem hiding this comment.
PR description says the only change is switching ingestion_dependency in this workflow, but this PR also adds multiple caching steps, changes the composite action to install ingestion[${{ inputs.ingestion_dependency }}], and increases Playwright workers in playwright.config.ts. Please update the PR description (or split the PR) so reviewers understand the full scope and rationale for each change.
| @@ -32,7 +32,7 @@ export default defineConfig({ | |||
| /* Retry on CI only */ | |||
| retries: process.env.CI ? 2 : 0, | |||
| /* Opt out of parallel tests on CI. */ | |||
There was a problem hiding this comment.
The comment says “Opt out of parallel tests on CI”, but workers: process.env.CI ? 4 : undefined explicitly configures parallel workers on CI (and fullyParallel: true is enabled). Please update the comment to reflect the intended CI concurrency rationale (or adjust the config to match the comment).
| /* Opt out of parallel tests on CI. */ | |
| /* Limit CI to a fixed number of workers to keep parallelism and resource usage predictable. */ |
| - name: Install Playwright Browsers | ||
| if: steps.cache-playwright.outputs.cache-hit != 'true' | ||
| run: npx playwright@1.57.0 install --with-deps |
There was a problem hiding this comment.
With the new cache/conditional, npx playwright@1.57.0 install --with-deps is skipped on cache hits. That changes the workflow semantics: --with-deps is also responsible for installing OS-level browser dependencies, which won’t happen on fresh runners if this step is skipped. Consider always running the deps installation (e.g., playwright install-deps) and only skipping the browser download, or just run install --with-deps unconditionally and rely on the cache to avoid re-downloading browsers.
| - name: Install Playwright Browsers | |
| if: steps.cache-playwright.outputs.cache-hit != 'true' | |
| run: npx playwright@1.57.0 install --with-deps | |
| - name: Install Playwright OS dependencies | |
| run: npx playwright@1.57.0 install-deps | |
| - name: Install Playwright Browsers | |
| if: steps.cache-playwright.outputs.cache-hit != 'true' | |
| run: npx playwright@1.57.0 install |
Jest test CoverageUI tests summary
|
🔴 Playwright Results — 2 failure(s), 18 flaky✅ 3447 passed · ❌ 2 failed · 🟡 18 flaky · ⏭️ 223 skipped
Genuine Failures (failed on all attempts)❌
|
Code Review 👍 Approved with suggestions 0 resolved / 5 findingsOptimizes PostgreSQL e2e workflow runtime by leveraging Playwright ingestion dependency with caching and tuning. Consider hardcoding the Playwright version in the cache key to prevent drift from package.json, and review the five minor findings across test data handling and servlet implementations. 💡 Edge Case: Hardcoded Playwright version in cache key may drift from package.json📄 .github/workflows/playwright-postgresql-e2e.yml:177 📄 .github/workflows/playwright-postgresql-e2e.yml:181 The Playwright browser cache key is hardcoded to Suggested fix💡 Edge Case: FailedTestCaseSampleData uses rowKey="name" but rows lack a name fieldIn Suggestion: Use the row index as the key instead, e.g., Suggested fix💡 Bug: tableCustomSQLQuery re-executes full SQL to fetch failed row sampleIn 💡 Edge Case: AddSqlQueryFormModal fetches table data even when testCase is undefinedIn Suggestion: Guard on Suggested fix💡 Quality: McpCallbackServlet errorSink ByteArrayOutputStream is never readIn Suggestion: Log 🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
Changes
Updates the PostgreSQL E2E workflow to use
playwrightas the ingestion dependency instead ofall.File Modified:
.github/workflows/playwright-postgresql-e2e.ymlingestion_dependencyfrom"all"to"playwright"(line 135)Impact
This optimization reduces CI build time and resource usage by only installing playwright-specific dependencies rather than all available ingestion dependencies for this E2E test workflow.