fix(tests): profile in setUpClass so test_list_entity_profiles has data - #31882
Merged
Conversation
pytest orders unittest TestCase methods alphabetically, so test_list_entity_profiles runs first -- before test_profiler_workflow has created any profile. The module sets TestLoader.sortTestMethodsUsing = None to force definition order, but that only affects unittest's own loader: pytest collects TestCase methods itself and ignores it. Verified against an unmodified checkout -- collection still yields list_entity_profiles first with that line in place, so it has never had any effect here. The test still passed, because it asserts on a global 24h window across every table rather than on data it owns, and hard-deleted tables used to leak their profiler rows into that window. #31556 stopped that leak (issue #27041), and the latent ordering bug surfaced: shard-2 now fails with "0 not greater than 0" on every PR that actually runs the ingestion integration matrix, blocking the merge queue. PRs that do not touch ingestion skip the matrix and report green, which is why this looked branch-specific rather than repo-wide. Run the profiler in setUpClass, where fixture data belongs, so no test depends on another's ordering. Drop the ineffective loader hack, and turn the `if profiles_all.entities:` guard into an assertion -- that guard swallowed an empty unfiltered listing and hid which of the two calls was actually empty, which is the signal needed to diagnose this.
Contributor
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
Contributor
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Khairajani
added
safe to test
IceS2
approved these changes
Aug 21, 2026
Khairajani
enabled auto-merge
August 21, 2026 11:10
Khairajani
added
safe to test
Code Review ✅ ApprovedMoves profiler workflow execution to setUpClass in TestSQAProfiler to ensure entity profiles exist before test execution, fixing test ordering dependencies. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
Contributor
✅ Playwright Results — workflow succeededValidated commit ✅ 110 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 26m 40s ⏱️ Max setup 4m 43s · max shard execution 10m 29s · max shard-job elapsed before upload 16m 17s · reporting 4s 🌐 216.15 requests/attempt · 1.79 app boots/UI scenario · 0.00% common-shard skew Optimization targets still in progress:
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
|
harsh-vador
added a commit
that referenced
this pull request
Aug 21, 2026
* fix(tests): profile in setUpClass so test_list_entity_profiles has data (#31882) pytest orders unittest TestCase methods alphabetically, so test_list_entity_profiles runs first -- before test_profiler_workflow has created any profile. The module sets TestLoader.sortTestMethodsUsing = None to force definition order, but that only affects unittest's own loader: pytest collects TestCase methods itself and ignores it. Verified against an unmodified checkout -- collection still yields list_entity_profiles first with that line in place, so it has never had any effect here. The test still passed, because it asserts on a global 24h window across every table rather than on data it owns, and hard-deleted tables used to leak their profiler rows into that window. #31556 stopped that leak (issue #27041), and the latent ordering bug surfaced: shard-2 now fails with "0 not greater than 0" on every PR that actually runs the ingestion integration matrix, blocking the merge queue. PRs that do not touch ingestion skip the matrix and report green, which is why this looked branch-specific rather than repo-wide. Run the profiler in setUpClass, where fixture data belongs, so no test depends on another's ordering. Drop the ineffective loader hack, and turn the `if profiles_all.entities:` guard into an assertion -- that guard swallowed an empty unfiltered listing and hid which of the two calls was actually empty, which is the signal needed to diagnose this. * fix(ui): make whole Domain and Data Product rows clickable (#31876) * fix(ui): make whole Domain and Data Product rows clickable * addressed comments * Added unit test for the fix * test(playwright): fix five AUT nightly flakes at their cause Collate's AUT nightly run 32476747983 retried 25 tests on each database lane. Fourteen of the distinct specs live here. These five have a cause the artifacts explain; the rest are listed below rather than guessed at. Lineage node clicks (LineageInteraction, both lanes) clickLineageNode clicked the node the moment the caller's getLineage wait returned. React Flow mounts nodes in its own layout pass after that, so the click auto-waited with no timeout of its own and the spec died as a bare "Test timeout of 60000ms exceeded" naming nothing. Assert the node is visible first. Ingestion wizard Next (ServiceIngestion) Creating the service triggers AutoPilot, whose toast renders bottom-center — over the wizard footer. The trace shows the click on next-button intercepted by the toast's own alert-message span, then auto-waiting until the test timed out. Wait for that toast to dismiss before advancing. Knowledge Center owner chip (ExplorePageRightPanel_KnowledgeCenter, both lanes) The Explore summary panel renders owners from the search document, refreshed asynchronously after the owner PATCH. A panel that rendered before the refresh will never show the chip, so the 60s wait was waiting on the wrong thing. Re-open the entity until it is there. Glossary hierarchy modal (GlossaryHierarchy) getByLabel('Select Parent') is page-scoped and also matches the control of a hierarchy modal an earlier step left in the DOM; the click then spent the whole test on a hidden element. Scope it to the modal and check it is visible and enabled first. Language switch (Glossary — Dutch) The menuitem click ran against an ant-dropdown mid-enter-animation, which is exactly what waitForAntdPopupToSettle exists for. Use it on both switches. Not addressed here, for lack of evidence rather than lack of interest: AdvancedSearchSuggestions, SearchSettings, ContextCenterArticles, ContextCenterMemories, ServiceEntity, GlossaryP2Tests and SSOConfiguration all failed as bare 60s timeouts with no anchor and passed on retry in seconds. Marking them slow would hide a regression just as easily as fix a flake, so they need a trace first. PlatformLineage and DataProductRename are already fixed on main by #31736. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(playwright): bound the wizard Next click instead of waiting the toast out Review point: waiting for the AutoPilot toast to detach is a no-op when the toast has not rendered yet, so a toast that appears a moment later still intercepts the click. Correct, and waiting for it to appear first is not the answer either — the toast is fired by the create call several steps earlier and auto-closes after 5s (showSuccessToast(..., 5000) in AddServicePage), so by the time this line runs it may equally have already closed. Any gate on its presence is wrong for one of the two orderings. Bound the click instead. Playwright retries an intercepted click for the whole action timeout, and 30s outlasts the toast in every ordering: not yet rendered, on screen now, or already gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Himanshu Khairajani <46777429+Khairajani@users.noreply.github.com> Co-authored-by: Anujkumar Yadav <anujf0510@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 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.
Fixes the
py-tests/py-tests-postgresshard-2 failure that is currently blocking the merge queue for every PR that touches ingestion.Root cause
test_list_entity_profilesruns first in its class — before the two tests that create the profiles it asserts on. pytest ordersunittest.TestCasemethods alphabetically (l<p), and the module-levelTestLoader.sortTestMethodsUsing = Noneintended to force definition order only affects unittest’s own loader; pytest collectsTestCasemethods itself and ignores it.Verified against an unmodified checkout — with that line present, collection still yields:
The CI artifacts agree: the junit XML records
test_list_entity_profilesexecuting first, with the other two passing afterwards.Why it passed until 2026-08-20
The test asserts on a global 24-hour window across every table rather than on data it owns. Hard-deleted tables used to leak their profiler rows into that window, so unrelated tests earlier in the shard left behind enough data to satisfy it.
#31556 fixed that leak (issue #27041). The server log from a failing run shows the new purge doing exactly its job:
26 such purges in a single run. With the leak gone, the window is genuinely empty when the listing test runs, and a test that was always logically broken finally started reporting it. #31556 is correct and should not be reverted.
Why it looked branch-specific
PRs that do not touch ingestion skip the integration matrix entirely and report green. Across ~40 recent
py-tests-postgresruns, every "pass" hadpython / Integration Testsskipped, and every run that actually executed it failed. So the queue looks healthy while rejecting all ingestion PRs — currentlyfix/expat-cve-2026-72522,fix/airflow-3.3.1-cve,taipei,pr-31761andayush-shah/ingestion-source-config-type.The change
setUpClassvia a newrun_profiler_workflows()helper, so profiles are class fixture data and no test depends on another’s ordering. This also removes one redundant profiler run.list_profiled_tables(), which was duplicated across both workflow tests.TestLoader.sortTestMethodsUsinghack rather than leave code that looks like it controls ordering.if profiles_all.entities:into an assertion. That guard swallowed an empty unfiltered listing and hid which of the two calls was empty — the one signal needed to diagnose this.test_profiler_workflow_w_globale_configstill re-profiles itself, since it must run after changing the global metric settings.Verification
ruff checkclean,ruff formatclean./v1/entity/profiles/tablereturned 6 table + 37 column profiles), confirming the API is fine and the defect is purely test ordering.Full integration run is on CI — it needs Docker testcontainers plus a live server.
Greptile Summary
Moves the initial profiler execution into class setup so profile-listing tests have deterministic fixture data regardless of test order.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Reviews (2): Last reviewed commit: "Merge branch 'main' into fix/profiler-te..." | Re-trigger Greptile