[SYNPY-1892] Integration test cuts - #1450
Merged
Merged
Conversation
configure_traces and configure_metrics now both call a new _build_resource_attributes helper, so configure_metrics gains service.instance.id and os.type is fixed to platform.system().lower() in both instead of the platform-indistinguishable os.name.
Wraps the async-job funnel in a synapse.async_job span, plus a submissions counter and a monotonic duration histogram recorded outside the retry loop so a view-not-available retry does not inflate the count, and on the failure path via try/finally.
Adds a file-handle upload counter and duration histogram inside the existing synapse.transfer.upload span, attributed by external_file_handle so the synapse_store=False (no S3 traffic) branch is separable from real uploads. Protected path core/upload/** touched under the approved override recorded in decisions.md (2026-08-14T20:15:36Z).
…code removal Adds telemetry_enabled and worker_telemetry_env to tests/integration/helpers.py so setup_otel honors the real SYNAPSE_INTEGRATION_TEST_OTEL_ENABLED truthiness (1/true/yes/on) and gives each pytest-xdist worker a distinct OTEL_SERVICE_INSTANCE_ID and appended OTEL_RESOURCE_ATTRIBUTES instead of colliding on one resource identity. Also deletes the dead active_span_processors list and its no-op force_flush loop in wrap_with_otel.
…col doc All eight remaining -n 8 sites move to -n 4 to match build.yml. Replaces CONTRIBUTING.md's OTel section, which documented the nonexistent SYNAPSE_OTEL_INTEGRATION_TEST_EXPORTER, with the real toggle (SYNAPSE_INTEGRATION_TEST_OTEL_ENABLED, strict 1/true/yes/on truthiness), OTEL_DEBUG_CONSOLE for credential-free verification, SYNAPSE_TEST_RUN_LABEL for measurement runs, and the two metric name pairs to query. Protected path .github/workflows/** touched under the approved override recorded in decisions.md (2026-08-14T20:15:36Z).
…c-job outcome, export-rejection guard - B1: root-span name is request.node.nodeid (was node.name), so parallel test attribution is unambiguous. - B2: synapse.async_job.submissions/.duration now carry an outcome attribute (success/timeout/error); the counter add moved into the existing finally alongside the histogram record, both from one attributes dict. - B3: ExportFailureRecorder + export_failure_summary in tests/integration/helpers.py capture rejected OTLP exports; conftest.py's setup_otel force-flushes both providers on teardown and the new pytest_sessionfinish/pytest_testnodedown/ pytest_terminal_summary hooks fail the session (even under xdist, via workeroutput) and print the rejection instead of leaving a silent green run. Verified live: Guard-serial and Guard-parallel runs against a deliberately malformed OTEL_EXPORTER_OTLP_HEADERS both exit non-zero and name the rejected status code (logs in .factory/tickets/SYNPY-1892/verification/r4/).
Sets synapse.file_handle.external on the metered upload_file_handle span, computed from synapse_store, so uploads(t, ext) is derivable from the trace.
…G_CONSOLE Rewrites the OTEL_DEBUG_CONSOLE paragraph: it only produces output for a serial `-s` run (default capture closes the stream before flush; execnet discards worker stdout under -n), points parallel verification at a real collector plus Slice 1's export-rejection guard, and documents that a rejected export fails the session non-zero instead of leaving it green. .env.example gains a commented SIGNOZ_API_KEY line and a note that a factory worktree has no .env of its own. Every command block is run verbatim with a captured log in .factory/tickets/SYNPY-1892/verification/r4/.
…st_load.py Two subcommands sharing one _query_range(payload) HTTP seam: `totals` (async-job submissions by request_type/outcome, uploads by external_file_handle, distinct service.instance.id, resource attributes) and `per-test` (joins root spans, async-job spans and upload spans on trace_id into a per-test load table, with cost/signature/unique/dominator/clear-contested scoring per requirements D2/D3, and an unattributed bucket per architecture B11). Live-verified against known-good data (no new dev-stack load): - `totals --label SYNPY-1892-verify-console` reproduces verification.md exactly (84/43/2/2/2 by request_type, 15 uploads). The plan cited this figure against `SYNPY-1892-verify-n4`, but that label's real totals are 130/59/8/2/2/86 (a larger corpus - more modules) - a plan citation error, not a script defect. - `per-test --label SYNPY-1892-verify-console` reproduces 53 root spans and 131/133 attributed async-job spans (2 unattributed) exactly. - `per-test`'s upload attribution (15/15 expected) cannot reproduce for this label: `synapse.file_handle.external` is Slice 2's new span attribute and does not exist on spans recorded before Slice 2 shipped, so the EXISTS-based query (needed to exclude unmetered multipart-upload spans on live data) finds zero matches here. Confirmed via raw query (15 spans without EXISTS, 0 with). Not a code defect - the acceptance target predates the attribute it queries.
…raw-trace paging Both defects were caught by Slice 6's own consistency checks against the full-suite measurement run, and both silently understate or double-count real load: - reduceTo "sum" adds each step interval's already-cumulative counter value, so a run spanning more than one step reads as a multiple of the truth (this 2h18m run: 954 async-job submissions and 1233 uploads reported, 487 and 617 actual). Short runs fit in one step, which is why Phase 0's figures reproduced exactly. Now reduceTo "max". - The v5 raw-trace endpoint returns nextCursor empty even on a full page, so the cursor loop stopped after 1000 rows and dropped 54 of 1054 test root spans; their async and upload spans then landed in the unattributed bucket, pushing it over the 10% blocker. Now pages by offset while pages come back full. Also excludes the legacy Synapse::_waitForAsync root span from the test-span denylist.
…s_async.py and test_materializedview_async.py Per measurement.md §4a's merge-clusters: share class-scoped fixtures across tests that build the same structure and only assert differently, cutting setup-load (async-job submissions / uploads) without dropping coverage. test_permissions_async.py: TestDeletePermissions' five complex_mixed_structure tests and TestAllEntityTypesPermissions' three read/dry-run tests now share one built structure each; the tests that perform real (non-dry-run) deletion keep their own fresh structure since sharing would leave stale ACL state for a sibling test. Fixes a leftover `stored_project` reference that no longer existed once three of those tests moved off the `stored_project` fixture. test_materializedview_async.py: TestMaterializedViewWithData's left/right/inner-join tests now share one pair of source tables (`join_source_tables`), and the two SELECT-only tests (`test_query_materialized_view`, `test_update_defining_sql`, `test_query_part_mask_async`) share one base table (`base_table_with_data`, now class-scoped). The two tests that mutate table rows (`test_materialized_view_reflects_table_updates`, `test_materialized_view_reflects_table_data_removal`) and the union test (different table shape) keep their own tables.
…ync.py Per measurement.md §4a's merge-clusters: TestProjectCopySync's test_copy_project_variations and test_sync_from_synapse now share one class-scoped shared_nested_project fixture. copy_async only reads the source project; sync_from_synapse_async repopulates the source project's local files/folders/annotations with equivalent values, so running one after the other against the same stored project is safe. TestProjectStore's test_store_project_with_files and test_store_project_with_nested_structure are left untouched: for both, Project.store_async storing files/folders is the behavior under test, not incidental setup a shared fixture could absorb.
…c.py Per feedback 0017 (Bryan): every File() built in this module exists only as a permission-bearing subject -- no test asserts on file content, size, or path. Replace all real uploads (path=utils.make_bogus_uuid_file()) with File(external_url=..., synapse_store=False): a FileEntity is still created, but the multipart upload is skipped. Reduces upload load on every test in the module that built a file this way, not just the tests already merged in the prior commit.
…aining async/operations/synapseutils test modules Applies the external_url/synapse_store=False technique (already used in test_permissions_async.py and test_project_async.py) file-wide across every test whose fixtures create a File but never assert on its content: the async model tests, the operations tests, and synapseutils copy/walk. Also removes two tests that wrote temp files into the repo root instead of using tempfile/schedule_for_cleanup (test_submission_bundle_async.py).
…c.py TestSyncFromSynapse None of these syncFromSynapse tests assert on downloaded file content, only on manifest/entity structure, so external_url/synapseStore=False file handles remove the real upload without weakening the assertions.
…ive sync Live Slice 7 verification proved this class's conversion to external_url/synapseStore=False wrong: syncFromSynapse's whole point is to download the file's content, and an external file handle pointing at a URL that was never actually written produces an empty manifest (folder-sync tests) or a failed download (single-file tests) rather than a passing sync. Unlike TestSyncToSynapse (the merge-cluster candidate, still applied-none) and the other files touched this round, TestSyncFromSynapse's real upload is load-bearing, not incidental. Reverts f94a482's change to this file only; that commit's rationale ("no content assertion") was wrong for this class.
…evaluation_async.py TestGetEvaluation, TestStoreEvaluation, TestDeleteEvaluation, and TestEvaluationAccess each defined an identical class-scoped test_project fixture that stored its own Project. None of these tests mutate the project, only evaluations pointed at it via content_source, so all four now use the conftest's existing session-scoped project_model fixture instead of creating four redundant projects.
… in test_submission_status_async.py This module was untouched by both the earlier merge-cluster and upload sweeps. TestSubmissionStatusRetrieval, TestSubmissionStatusUpdates, and TestSubmissionStatusCancellation each defined identical class-scoped test_evaluation and test_file fixtures - none of these classes mutate the evaluation or file itself, only the Submission/SubmissionStatus objects created against them - so those three now share one module-scoped test_evaluation and test_file. Both file fixtures also switch from a real tempfile upload to an external_url handle since no test asserts on file content. TestSubmissionStatusBulkOperations keeps its own isolated test_evaluation: its tests call get_all_submission_statuses_async's default page (limit=10) and assert every submission they just created is on that page, so sharing the module-level evaluation would let the other classes' submissions push theirs off the page. Verified live: without the isolated fixture, test_get_all_submission_statuses failed this way. Its test_files fixture also switches to external_url file handles.
…ion_async.py TestSubmissionCreationAsync, TestSubmissionRetrievalAsync, and TestSubmissionDeletionAsync each defined identical class-scoped test_project and test_evaluation fixtures. None of these classes mutate the project or evaluation, only the Submission objects created against them, so all three now share one module-level test_evaluation on the existing project_model fixture. TestSubmissionRetrievalAsync's assertions walk an async generator (get_evaluation_submissions_async), not a default-limited page, so sharing the evaluation's submission count across classes is safe here (unlike SubmissionStatus's get_all, fixed in the previous commit). Also converts the three per-class test_file fixtures (Retrieval, Deletion) that still uploaded a real temp file to the external_url pattern already used elsewhere in this file, since no test asserts on file content. TestSubmissionCancelAsync's test_project/test_evaluation/test_file fixtures are deleted outright: its only test takes no fixture parameters, so pytest never instantiated them - dead code, not merely duplication.
…ion_bundle_async.py TestSubmissionBundleRetrievalAsync and TestSubmissionBundleDataIntegrityAsync each defined identical class-scoped test_project and test_evaluation fixtures. Neither class asserts on the evaluation's total submission count, only that its own submissions are present via async-generator pagination, so both now share one module-level test_evaluation on the existing project_model fixture. TestSubmissionBundleEdgeCasesAsync keeps its own isolated test_evaluation: its tests assert the evaluation has zero submissions, so it cannot share the module-level one that the other two classes submit to. It still drops its own redundant project creation in favor of project_model, since nothing in this class asserts the project itself is empty. Its test_file and test_submission fixtures are deleted outright - none of its five tests take either as a parameter, so pytest never instantiated them.
… clock The four prompting tests in this module accounted for 19,561s of the 22,904s of test time in the SYNPY-1892-full-20260817 corpus - 85.4% of the suite - and `--dist loadscope` pins the module to a single worker, so that time is serial and the suite cannot finish before it does. The shared ASYNC_JOB_TIMEOUT_SEC of 600 did not bound them. `get_job_async` resets its clock on every progress message, so its timeout is a no-progress budget rather than a total one and a job that keeps reporting progress runs without limit; one prompt was measured at 1256s and ended in a 500 rather than a timeout. `--reruns 3` then repeated each one four times, and every execution in the corpus ran to full length, so no retry ever succeeded. Each prompt now carries a module-local no-progress timeout of 120s, matching the client's own default, inside a hard 300s wall-clock deadline. The module opts out of reruns, since an agent that will not answer is an outage rather than flake. Measured on the dev stack: the same test now fails in 301.54s instead of 1256s, and runs once under `--reruns 3` instead of four times.
…s to real uploads sync_from_synapse_async does not populate annotations/activity for an external_url file handle, so these two tests need real file content on disk. Other tests in the class (structure-only checks) keep external_url.
…xture shared_nested_project now hands each test its own deep copy of the class-scoped build, so test_copy_project_variations and test_sync_from_synapse no longer depend on pytest's definition-order execution to avoid mutating each other's state.
…TING.md Records the rule found the hard way three times this ticket: an external_url/synapse_store=False File is fine for existence-only checks but breaks any test that reads the file back through sync_from_synapse, a download, an md5 comparison, or a manifest/annotation/provenance round-trip.
…est_table_async.py TestTableSnapshot: 4 tests collapsed into 1 sequential test over a shared table, asserting each snapshot version against an incrementing counter instead of a hardcoded 1. TestDeleteRows: 5 tests kept separate but now share a class-scoped table populated by a single store_rows_async call. Each test operates on its own disjoint row group (g1-g4) so the shared server-side state cannot cross-contaminate between tests regardless of run order; the exception test targets nonexistent row ids and touches no real rows.
Seven classes in test_wiki_async.py each created their own Project solely to own a root wiki page. Synapse allows any entity to own a wiki, so each class now creates a Folder inside the session-shared project_model fixture instead, cutting 12 Project creations per module run down to 0 (project_model already exists) plus 8 Folders. Verified folder-owned wikis behave identically for CRUD, attachments, markdown, versioning, header/order-hint, and tree copy (folder->folder).
…file The class-scoped test_file fixture only needs a parent container for its real file upload, so it now creates a Folder inside the session-shared project_model fixture instead of its own Project.
…TaskSetActiveGridSessionAsync tests
…ema across TestRecordSetGetDetailedValidationResultsAsync tests
… separate entities in test_virtual_table_data_queries
…t in test_docker_async.py
…sts in test_dataset_async.py Two class-scoped fixtures (shared_datasets_with_file, shared_datasets_without_file) replace 5 per-test Dataset creations with 4 built once for the class; the create_dataset/create_file_instance helpers they replaced are now unused and removed. TestDataset is left unchanged: every test there mutates the Dataset's own columns/items/version count, which other tests' exact assertions would observe if shared.
…ts in test_entityview_async.py shared_folder_with_files (class-scoped) replaces 4 of 5 setup_files_in_folder calls, cutting folder/File-entity creation from 5 folders/12 files to 2 folders/8 files. test_update_rows_and_annotations keeps its own dedicated folder+files since it writes annotations onto the files, which the other tests' views (scoped to the same folder) would otherwise observe.
…ifications tests in test_submissionview_async.py shared_evaluations (class-scoped) replaces 3 per-test Evaluation creations with 2 built once for the class; neither test submits to, deletes, or queries submission content against them, only reads their id as a scope_ids target. Other classes in this module are left unchanged: TestSubmissionViewWithSubmissions tests each need their own dedicated evaluation, since a submissionview's query sees all submissions in its scope -- sharing the evaluation would let one test's submissions appear in the other's exact-count assertions.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces integration-test runtime and API/resource churn by reusing read-only Synapse resources across tests and avoiding unnecessary file uploads, while also improving OpenTelemetry (OTel) observability and documentation for integration test execution.
Changes:
- Refactors many integration tests to share Projects/Evaluations/Tables/etc. via broader-scope fixtures, and replaces many real file uploads with
external_url/synapse_store=FalseFileEntities where content is not needed. - Strengthens OTel integration-test setup: opt-in truthiness check, per-xdist-worker resource identity, metrics enablement, and failing the run on OTLP export rejections.
- Adds OTel metric instruments for async job submissions/durations and file-handle upload counts/durations; refactors OTel resource-attribute construction to be shared between traces and metrics.
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/synapseutils/test_synapseutils_walk.py | Avoids real uploads by using external-URL FileEntities in walk tests. |
| tests/integration/synapseutils/test_synapseutils_copy.py | Uses external-URL FileEntity as Link target to avoid upload. |
| tests/integration/synapseclient/operations/async/test_utility_operations_async.py | Uses external-URL File model for name-based lookup test. |
| tests/integration/synapseclient/operations/async/test_factory_operations_store_async.py | Uses external-URL File model in store factory tests to avoid uploads. |
| tests/integration/synapseclient/operations/async/test_download_list_operations_async.py | Updates concurrency note to match new -n 4 default. |
| tests/integration/synapseclient/operations/async/test_delete_operations_async.py | Replaces uploaded files with external-URL files across delete scenarios. |
| tests/integration/synapseclient/models/synchronous/test_sync_wrapper_smoke.py | Uses external-URL File model for sync wrapper smoke tests. |
| tests/integration/synapseclient/models/async/test_wiki_async.py | Reuses session project via folder owners to avoid per-class projects; reduces entity creation. |
| tests/integration/synapseclient/models/async/test_virtualtable_async.py | Reuses a single VirtualTable and updates defining SQL instead of creating multiple tables. |
| tests/integration/synapseclient/models/async/test_table_async.py | Shares a single table for multiple delete/snapshot scenarios to reduce setup calls. |
| tests/integration/synapseclient/models/async/test_submissionview_async.py | Shares evaluations and uses external-URL FileEntities for submissions where content is irrelevant. |
| tests/integration/synapseclient/models/async/test_submission_status_async.py | Introduces module-scoped shared Evaluation/File fixtures; reduces temp-file uploads. |
| tests/integration/synapseclient/models/async/test_submission_bundle_async.py | Shares Evaluation across classes and uses external-URL files to avoid local file writes/uploads. |
| tests/integration/synapseclient/models/async/test_submission_async.py | Shares evaluation and uses external-URL files across submission tests. |
| tests/integration/synapseclient/models/async/test_storable_container_async.py | Uses external-URL files for container tests where content isn’t needed. |
| tests/integration/synapseclient/models/async/test_recordset_async.py | Widens fixture scope to share schema/recordset setup and reduces repeated work. |
| tests/integration/synapseclient/models/async/test_project_async.py | Shares a nested project structure across tests; replaces uploads with external-URL files where safe. |
| tests/integration/synapseclient/models/async/test_permissions_async.py | Switches permission tests to external-URL files to avoid uploads while preserving coverage. |
| tests/integration/synapseclient/models/async/test_materializedview_async.py | Shares base/join source tables via class-scoped fixtures to reduce repeated setup. |
| tests/integration/synapseclient/models/async/test_json_schema_async.py | Uses external-URL files where only entity existence matters; reduces uploads. |
| tests/integration/synapseclient/models/async/test_grid_async.py | Uses external-URL files for EntityView scope setup to avoid uploads. |
| tests/integration/synapseclient/models/async/test_form_async.py | Reuses session project via folder parent while keeping a real upload where download/content is required. |
| tests/integration/synapseclient/models/async/test_folder_async.py | Uses external-URL files broadly; keeps real uploads for annotation/provenance manifest tests. |
| tests/integration/synapseclient/models/async/test_evaluation_async.py | Reuses project_model instead of per-class projects to reduce setup cost. |
| tests/integration/synapseclient/models/async/test_entityview_async.py | Adds shared folder+files fixture and uses external-URL file handle reuse to reduce file creation. |
| tests/integration/synapseclient/models/async/test_docker_async.py | Reuses session project and ensures repo names are unique per test/fixture invocation. |
| tests/integration/synapseclient/models/async/test_dataset_async.py | Shares datasets via class fixtures and replaces uploads with external-URL files when content not needed. |
| tests/integration/synapseclient/models/async/test_curation_async.py | Shares folder/view/grid fixtures for class-scoped read-only access patterns. |
| tests/integration/synapseclient/models/async/test_agent_async.py | Adds hard wall-clock deadlines and skips agent tests due to dev timeouts. |
| tests/integration/synapseclient/models/async/test_activity_async.py | Replaces uploads with external-URL files for activity tests where content isn’t needed. |
| tests/integration/helpers.py | Adds OTel helpers for enablement truthiness and per-worker env shaping; adds OTLP export failure capture utility. |
| tests/integration/conftest.py | Enables OTel metrics, per-worker resource identity, captures exporter failures, and improves per-test span naming. |
| tests/CLAUDE.md | Updates testing/OTel guidance and review date to match new integration-test practices. |
| synapseclient/models/mixins/asynchronous_job.py | Adds OTel metrics + span around async job submission/wait path. |
| synapseclient/core/upload/upload_functions_async.py | Adds OTel metrics for upload/external-file-handle paths and records duration. |
| synapseclient/core/otel_config.py | Refactors shared resource attribute building; aligns os.type with platform values; reuses across traces/metrics. |
| CONTRIBUTING.md | Updates integration test parallelism guidance (-n 4) and expands OTel usage/diagnostics documentation. |
| CLAUDE.md | Updates recommended integration test invocation (-n 4) and review date. |
| .github/workflows/validate-release.yml | Updates integration test xdist worker count to -n 4. |
| .github/workflows/build.yml | Updates integration test xdist worker count to -n 4. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
|
@BryanFauble You're now skipping some integration tests |
andrewelamb
approved these changes
Aug 21, 2026
- Fix misleading comment on max_wait_time (scales with timeout, not fixed 5 min) - Replace fixed asyncio.sleep() with wait_for_condition polling in virtual table tests - Fix f-string + concatenation in test_wiki_async.py folder names - Remove unused Any import in tests/integration/helpers.py
BryanFauble
enabled auto-merge (squash)
August 21, 2026 20:38
BryanFauble
disabled auto-merge
August 21, 2026 21:15
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
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.
Problem:
Solution:
Testing: