[SYNPY-1295] Trim down integration tests #1199

BryanFauble · 2025-04-25T23:37:48Z

Problem:

Integration test runs are taking longer and longer, after talking with Xa we found out:

Bottlenecks looking at the queues are download_csv_from_table (160+), query (130+) and table_view (6K+).
We have a lot of calls to 'GET /user/#/team'.

Solution:

Use github copilot agent mode to trim down the number of required tests and combine similar logic
The number of tests was reduced from 598 to 440 (-158). Although some tests significantly expanded, this represents a significant reduction.
Deleted a significant build-up of data with: https://gist.github.com/BryanFauble/49342e30f13c33e037f5cc813d4c7ab4

Testing:

I ran the tests locally and found that these tests all seemed to pass
Will also be reviewing the integration test run in github
Test coverage is marginally worse (Decreasing around 4%)

With changes:   lines-valid="28400" lines-covered="10092" line-rate="0.3554"
Before changes: lines-valid="29975" lines-covered="11736" line-rate="0.3915"

…ts and combine similar logic

Copilot

Pull Request Overview

This PR aims to trim down integration tests by consolidating similar logic and reducing redundant test cases to speed up execution.

Consolidated file creation and activity verification logic via helper functions.
Updated both synchronous and asynchronous tests to remove redundant client parameter passing.
Adjusted team tests to use common property verification and reduce test bloat.

Reviewed Changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.

File	Description
tests/integration/synapseclient/models/synchronous/test_activity.py	Introduced helper methods for file creation and activity verification; removed explicit “synapse_client” parameters in some calls.
tests/integration/synapseclient/models/async/test_team_async.py	Reorganized team lifecycle tests to use a common property verification method and added checks for team membership/invitations.
tests/integration/synapseclient/models/async/test_activity_async.py	Paralleled synchronous tests by creating async helper methods and ensuring proper activity lifecycle validation.

…sistency

BryanFauble · 2025-04-29T17:54:48Z

synapseclient/client.py

-                    **STANDARD_RETRY_PARAMS,
+                    **{**STANDARD_RETRY_PARAMS, "retries": 2},


I found this while working on this: If the endpoints that you set are incorrect, like trying to use https://auth-dev.dev.sagebase.org/auth/v1 instead of the correct https://repo-dev.dev.sagebase.org/auth/v1 for the authEndpoint the withRetry loop will be stuck here because the error is retried instead of failing out.

The error we want to let this fail out with is:

requests.exceptions.ConnectionError: HTTPSConnectionPool(host='auth-dev.dev.sagebase.org', port=443): Max retries exceeded with url: /auth/v1 (Caused by NewConnectionError('<urllib3.connection.HTTPSConnection object at 0x7f225f99dc10>: Failed to establish a new connection: [Errno -2] Name or service not known'))

Script to re-create this:

from synapseclient import Synapse DEV_ENDPOINTS_SUCCESS = { "repoEndpoint": "https://repo-dev.dev.sagebase.org/repo/v1", "authEndpoint": "https://repo-prod.dev.sagebase.org/auth/v1", "fileHandleEndpoint": "https://repo-dev.dev.sagebase.org/file/v1", "portalEndpoint": "https://dev.synapse.org/", } DEV_ENDPOINTS_FAIL = { "repoEndpoint": "https://repo-dev.dev.sagebase.org/repo/v1", "authEndpoint": "https://auth-dev.dev.sagebase.org/auth/v1", "fileHandleEndpoint": "https://repo-dev.dev.sagebase.org/file/v1", "portalEndpoint": "https://dev.synapse.org/", } syn = Synapse() syn.setEndpoints(**DEV_ENDPOINTS_SUCCESS) print(f"Set endpoints successfully: {DEV_ENDPOINTS_SUCCESS}") syn.setEndpoints(**DEV_ENDPOINTS_FAIL) print(f"Set endpoints unsuccessfully: {DEV_ENDPOINTS_FAIL}")

…ling in submission view tests to use evaluation objects directly

…ntainability

BryanFauble · 2025-04-29T19:08:24Z

.github/workflows/build.yml

+          # Setup ignore patterns based on Python version
+          IGNORE_FLAGS="--ignore=tests/integration/synapseclient/test_command_line_client.py"
+
+          if [ "${{ matrix.python }}" == "3.9" ]; then
+            # For min Python version, ignore async tests
+            IGNORE_FLAGS="$IGNORE_FLAGS --ignore=tests/integration/synapseclient/models/async/"
+            echo "Running integration tests for Min Python version (3.9) - ignoring async tests"
+          elif [ "${{ matrix.python }}" == "3.13" ]; then
+            # For max Python version, ignore synchronous tests
+            IGNORE_FLAGS="$IGNORE_FLAGS --ignore=tests/integration/synapseclient/models/synchronous/"
+            echo "Running integration tests for Max Python version (3.13) - ignoring synchronous tests"
+          fi
+
          # use loadscope to avoid issues running tests concurrently that share scoped fixtures
-          pytest -sv --reruns 3 --cov-append --cov=. --cov-report xml tests/integration -n 8 --ignore=tests/integration/synapseclient/test_command_line_client.py --dist loadscope
+          pytest -sv --reruns 3 --cov-append --cov=. --cov-report xml tests/integration -n 8 $IGNORE_FLAGS --dist loadscope


This will let us get away with running fewer integration tests while making sure that they all get ran on the same OS (But different python versions)

BWMac · 2025-04-29T19:22:05Z

@BryanFauble Do you have a summary of how many total API requests were cut out of the integration testing process by the reduction in tests?

BryanFauble · 2025-04-29T19:37:31Z

@BryanFauble Do you have a summary of how many total API requests were cut out of the integration testing process by the reduction in tests?

@BWMac That would be a great metric, but I don't have the numbers for this no. We could probably use snowflake to look at the averages once we get the data tomorrow.

BWMac

LGTM! Just one question

BWMac · 2025-04-29T19:29:19Z

tests/integration/synapseclient/models/async/test_entityview_async.py

+        test_cases = [
+            {
+                "name": "default_columns",
+                "description": "EntityView with default columns",
+                "columns": None,
+                "include_default_columns": True,
+                "expected_column_count": None,  # Will be set after getting default columns
+            },
+            {
+                "name": "single_column",
+                "description": "EntityView with a single column",
+                "columns": [Column(name="test_column", column_type=ColumnType.STRING)],
+                "include_default_columns": False,
+                "expected_column_count": 1,
+            },
+            {
+                "name": "multiple_columns",
+                "description": "EntityView with multiple columns",
+                "columns": [
+                    Column(name="test_column", column_type=ColumnType.STRING),
+                    Column(name="test_column2", column_type=ColumnType.INTEGER),
+                ],
+                "include_default_columns": False,
+                "expected_column_count": 2,
+            },
+        ]


Did you do this instead of pytest parametrize so you only have to make the api call to get the default columns once?

I didn't make the changes in this case (Github copilot did). I sparsely checked the work and didn't find anything super crazy.

so you only have to make the api call to get the default columns once?

This sounds like a good reason why copilot would have done it like this, although another approach could have been to extract the default columns to a fixture used for the module, and used pytest parametrize. 🤷

Use github copilot agent mode to trim down the number of required tes…

13359b9

…ts and combine similar logic

BryanFauble requested a review from Copilot April 25, 2025 23:38

Copilot AI reviewed Apr 25, 2025

View reviewed changes

Update retry parameters and refactor async test setup methods for con…

43286a6

…sistency

BryanFauble commented Apr 29, 2025

View reviewed changes

BryanFauble added 5 commits April 29, 2025 11:22

Split async/sync tests so both aren't run and Refactor cleanup schedu…

37c8f52

…ling in submission view tests to use evaluation objects directly

Refactor test ignore patterns to use flags for better clarity and mai…

7fde915

…ntainability

Merge branch 'develop' into synpy-1595-trim-integration-tests

cd30855

Add test trimming logic to virtual table

44e1f5f

Bump python dependency cache

59e5b9c

BryanFauble commented Apr 29, 2025

View reviewed changes

BryanFauble marked this pull request as ready for review April 29, 2025 19:08

BryanFauble requested a review from a team as a code owner April 29, 2025 19:08

BryanFauble requested a review from xschildw April 29, 2025 19:10

Increase wait time for name-based team retrieval in async and sync tests

b3b3e75

BWMac approved these changes Apr 29, 2025

View reviewed changes

BryanFauble mentioned this pull request Apr 29, 2025

Patch integrations tests for multi-profile login #1198

Merged

BryanFauble merged commit f348ee7 into develop Apr 29, 2025
28 checks passed

BryanFauble deleted the synpy-1595-trim-integration-tests branch April 29, 2025 20:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SYNPY-1295] Trim down integration tests #1199

[SYNPY-1295] Trim down integration tests #1199

Uh oh!

BryanFauble commented Apr 25, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

BryanFauble Apr 29, 2025

Uh oh!

BryanFauble Apr 29, 2025

Uh oh!

BryanFauble Apr 29, 2025

Uh oh!

BWMac commented Apr 29, 2025 •

edited

Loading

Uh oh!

BryanFauble commented Apr 29, 2025

Uh oh!

BWMac left a comment

Uh oh!

BWMac Apr 29, 2025

Uh oh!

BryanFauble Apr 29, 2025

Uh oh!

Uh oh!

Uh oh!

		**STANDARD_RETRY_PARAMS,
		{STANDARD_RETRY_PARAMS, "retries": 2},

[SYNPY-1295] Trim down integration tests #1199

[SYNPY-1295] Trim down integration tests #1199

Uh oh!

Conversation

BryanFauble commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem:

Solution:

Testing:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

BryanFauble Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

BryanFauble Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

BryanFauble Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

BWMac commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BryanFauble commented Apr 29, 2025

Uh oh!

BWMac left a comment

Choose a reason for hiding this comment

Uh oh!

BWMac Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

BryanFauble Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BryanFauble commented Apr 25, 2025 •

edited

Loading

BWMac commented Apr 29, 2025 •

edited

Loading