From 9ab076ab9868cc9ccfc5da6d05a25e0510d541c4 Mon Sep 17 00:00:00 2001 From: Jordan Sanders Date: Fri, 2 May 2025 08:52:10 -0500 Subject: [PATCH 1/6] Fix batch submission This was returning early meaning only the first batch was being submitted. This caused truncation in test collection for large test suites. --- src/buildkite_test_collector/collector/api.py | 13 ++++++++----- .../buildkite_test_collector/collector/test_api.py | 14 ++++++++------ 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/src/buildkite_test_collector/collector/api.py b/src/buildkite_test_collector/collector/api.py index 1c49b0b..181ed29 100644 --- a/src/buildkite_test_collector/collector/api.py +++ b/src/buildkite_test_collector/collector/api.py @@ -1,6 +1,6 @@ """Buildkite Test Analytics API""" -from typing import Optional +from typing import Any, Generator, Optional from os import environ import traceback from requests import post, Response @@ -9,7 +9,7 @@ from ..pytest_plugin.logger import logger -def submit(payload: Payload, batch_size=100) -> Optional[Response]: +def submit(payload: Payload, batch_size=100) -> Generator[Optional[Response], Any, Any]: """Submit a payload to the API""" token = environ.get("BUILDKITE_ANALYTICS_TOKEN") api_url = environ.get("BUILDKITE_ANALYTICS_API_URL", "https://analytics-api.buildkite.com/v1") @@ -17,8 +17,9 @@ def submit(payload: Payload, batch_size=100) -> Optional[Response]: if not token: logger.warning("No `BUILDKITE_ANALYTICS_TOKEN` environment variable present") + yield None - if token: + else: try: for payload_slice in payload.into_batches(batch_size): response = post(api_url + "/uploads", @@ -29,14 +30,16 @@ def submit(payload: Payload, batch_size=100) -> Optional[Response]: }, timeout=60) response.raise_for_status() - return response + yield response except InvalidHeader as error: logger.warning("Invalid `BUILDKITE_ANALYTICS_TOKEN` environment variable") logger.warning(error) + yield None except HTTPError as err: logger.warning("Failed to uploads test results to buildkite") logger.warning(err) + yield None except Exception: # pylint: disable=broad-except error_message = traceback.format_exc() logger.warning(error_message) - return None + yield None diff --git a/tests/buildkite_test_collector/collector/test_api.py b/tests/buildkite_test_collector/collector/test_api.py index 57e188f..327665e 100644 --- a/tests/buildkite_test_collector/collector/test_api.py +++ b/tests/buildkite_test_collector/collector/test_api.py @@ -141,14 +141,16 @@ def test_submit_with_large_payload_batches_requests(successful_test, failed_test payload = payload.push_test_data(successful_test) payload = payload.push_test_data(failed_test) - result = submit(payload, batch_size=1) + results = [response for response in submit(payload, batch_size=1)] + assert len(results) == 2 - assert result.status_code >= 200 - assert result.status_code < 300 + for result in results: + assert result.status_code >= 200 + assert result.status_code < 300 - json = result.json() - assert len(json["errors"]) == 0 - assert json['queued'] == 1 + json = result.json() + assert len(json["errors"]) == 0 + assert json['queued'] == 1 @responses.activate def test_api_url_override(successful_test): From 4e1c01dda780d3ab89116810d06793597a6f953d Mon Sep 17 00:00:00 2001 From: Jordan Sanders Date: Fri, 2 May 2025 13:53:07 -0500 Subject: [PATCH 2/6] Wrap try/except in for loop Another small thing we noticed - even with the yield logic, it would have short circuited on the first exception. Now, it continues looping over the whole bach even if one of the requests hits an exception. --- src/buildkite_test_collector/collector/api.py | 28 +++++++++---------- .../collector/test_api.py | 1 + 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/buildkite_test_collector/collector/api.py b/src/buildkite_test_collector/collector/api.py index 181ed29..0f3f9ef 100644 --- a/src/buildkite_test_collector/collector/api.py +++ b/src/buildkite_test_collector/collector/api.py @@ -20,8 +20,8 @@ def submit(payload: Payload, batch_size=100) -> Generator[Optional[Response], An yield None else: - try: - for payload_slice in payload.into_batches(batch_size): + for payload_slice in payload.into_batches(batch_size): + try: response = post(api_url + "/uploads", json=payload_slice.as_json(), headers={ @@ -31,15 +31,15 @@ def submit(payload: Payload, batch_size=100) -> Generator[Optional[Response], An timeout=60) response.raise_for_status() yield response - except InvalidHeader as error: - logger.warning("Invalid `BUILDKITE_ANALYTICS_TOKEN` environment variable") - logger.warning(error) - yield None - except HTTPError as err: - logger.warning("Failed to uploads test results to buildkite") - logger.warning(err) - yield None - except Exception: # pylint: disable=broad-except - error_message = traceback.format_exc() - logger.warning(error_message) - yield None + except InvalidHeader as error: + logger.warning("Invalid `BUILDKITE_ANALYTICS_TOKEN` environment variable") + logger.warning(error) + yield None + except HTTPError as err: + logger.warning("Failed to uploads test results to buildkite") + logger.warning(err) + yield None + except Exception: # pylint: disable=broad-except + error_message = traceback.format_exc() + logger.warning(error_message) + yield None diff --git a/tests/buildkite_test_collector/collector/test_api.py b/tests/buildkite_test_collector/collector/test_api.py index 327665e..8b5dc03 100644 --- a/tests/buildkite_test_collector/collector/test_api.py +++ b/tests/buildkite_test_collector/collector/test_api.py @@ -145,6 +145,7 @@ def test_submit_with_large_payload_batches_requests(successful_test, failed_test assert len(results) == 2 for result in results: + assert result assert result.status_code >= 200 assert result.status_code < 300 From 66a04a0498f47edaf778bd953a7c1c68f1c4da40 Mon Sep 17 00:00:00 2001 From: Jordan Sanders Date: Fri, 2 May 2025 16:50:42 -0500 Subject: [PATCH 3/6] Fixup generator tests --- tests/buildkite_test_collector/collector/test_api.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/tests/buildkite_test_collector/collector/test_api.py b/tests/buildkite_test_collector/collector/test_api.py index 8b5dc03..161679b 100644 --- a/tests/buildkite_test_collector/collector/test_api.py +++ b/tests/buildkite_test_collector/collector/test_api.py @@ -16,14 +16,14 @@ def test_submit_with_missing_api_key_environment_variable_returns_none(): with mock.patch.dict(os.environ, {"CI": "true", "BUILDKITE_ANALYTICS_TOKEN": ""}): payload = Payload.init(detect_env()) - assert submit(payload) is None + assert next(submit(payload)) is None def test_submit_with_invalid_api_key_environment_variable_returns_none(): with mock.patch.dict(os.environ, {"CI": "true", "BUILDKITE_ANALYTICS_TOKEN": "\n"}): payload = Payload.init(detect_env()) - assert submit(payload) is None + assert next(submit(payload)) is None @responses.activate @pytest.mark.skipif(sys.version_info < (3, 9), reason="requires python3.9 or higher") @@ -84,7 +84,8 @@ def test_submit_with_payload_returns_an_api_response(successful_test): payload = payload.push_test_data(successful_test) - result = submit(payload) + result = next(submit(payload)) + assert result assert result.status_code >= 200 assert result.status_code < 300 @@ -107,7 +108,7 @@ def test_submit_with_bad_response(successful_test): payload = payload.push_test_data(successful_test) - result = submit(payload) + result = next(submit(payload)) assert result is None @@ -175,7 +176,8 @@ def test_api_url_override(successful_test): payload = payload.push_test_data(successful_test) - result = submit(payload) + result = next(submit(payload)) + assert result assert result.status_code >= 200 assert result.status_code < 300 From 55ecee325846e83d699cb3fe395ca86349939881 Mon Sep 17 00:00:00 2001 From: Jordan Sanders Date: Fri, 2 May 2025 16:54:55 -0500 Subject: [PATCH 4/6] Remaining generator fixes --- src/buildkite_test_collector/pytest_plugin/__init__.py | 4 ++-- tests/buildkite_test_collector/collector/test_api.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/buildkite_test_collector/pytest_plugin/__init__.py b/src/buildkite_test_collector/pytest_plugin/__init__.py index 2604cf3..9ae81cc 100644 --- a/src/buildkite_test_collector/pytest_plugin/__init__.py +++ b/src/buildkite_test_collector/pytest_plugin/__init__.py @@ -47,11 +47,11 @@ def pytest_unconfigure(config): # When xdist is not installed, or when it's installed and not enabled if not xdist_enabled: - submit(plugin.payload) + [response for response in submit(plugin.payload)] # When xdist is activated, we want to submit from worker thread only, because they have access to tag data if xdist_enabled and is_xdist_worker: - submit(plugin.payload) + [response for response in submit(plugin.payload)] # We only want a single thread to write to the json file. # When xdist is enabled, that will be the controller thread. diff --git a/tests/buildkite_test_collector/collector/test_api.py b/tests/buildkite_test_collector/collector/test_api.py index 161679b..2f5e529 100644 --- a/tests/buildkite_test_collector/collector/test_api.py +++ b/tests/buildkite_test_collector/collector/test_api.py @@ -39,7 +39,7 @@ def test_submit_with_payload_timeout_captures_ConnectTimeout_error(capfd, succes payload = payload.push_test_data(successful_test) - result = submit(payload) + result = next(submit(payload)) captured = capfd.readouterr() assert captured.err.startswith("buildkite-test-collector - WARNING -") @@ -59,7 +59,7 @@ def test_submit_with_payload_timeout_captures_ReadTimeout_error(capfd, successfu payload = payload.push_test_data(successful_test) - result = submit(payload) + result = next(submit(payload)) captured = capfd.readouterr() assert captured.err.startswith("buildkite-test-collector - WARNING -") From c64507f480cad8d8b732425cdf940c666d088f04 Mon Sep 17 00:00:00 2001 From: Jordan Sanders Date: Mon, 5 May 2025 08:48:03 -0500 Subject: [PATCH 5/6] Fix pylint --- src/buildkite_test_collector/pytest_plugin/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/buildkite_test_collector/pytest_plugin/__init__.py b/src/buildkite_test_collector/pytest_plugin/__init__.py index 9ae81cc..608db60 100644 --- a/src/buildkite_test_collector/pytest_plugin/__init__.py +++ b/src/buildkite_test_collector/pytest_plugin/__init__.py @@ -47,11 +47,11 @@ def pytest_unconfigure(config): # When xdist is not installed, or when it's installed and not enabled if not xdist_enabled: - [response for response in submit(plugin.payload)] + list(submit(plugin.payload)) # When xdist is activated, we want to submit from worker thread only, because they have access to tag data if xdist_enabled and is_xdist_worker: - [response for response in submit(plugin.payload)] + list(submit(plugin.payload)) # We only want a single thread to write to the json file. # When xdist is enabled, that will be the controller thread. From c0c8d8fe8a66e824a0865415cbcf52ab31c00253 Mon Sep 17 00:00:00 2001 From: Jordan Sanders Date: Mon, 5 May 2025 08:53:00 -0500 Subject: [PATCH 6/6] Assert later batches submit if earlier ones fail --- .../collector/test_api.py | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/tests/buildkite_test_collector/collector/test_api.py b/tests/buildkite_test_collector/collector/test_api.py index 2f5e529..8dee412 100644 --- a/tests/buildkite_test_collector/collector/test_api.py +++ b/tests/buildkite_test_collector/collector/test_api.py @@ -154,6 +154,47 @@ def test_submit_with_large_payload_batches_requests(successful_test, failed_test assert len(json["errors"]) == 0 assert json['queued'] == 1 +@responses.activate +def test_submit_with_batches_and_errors(capfd, successful_test, failed_test): + responses.add( + responses.POST, + "https://analytics-api.buildkite.com/v1/uploads", + body=ConnectTimeout("Error")) + responses.add( + responses.POST, + "https://analytics-api.buildkite.com/v1/uploads", + json={'id': str(uuid4()), + 'run_id': str(uuid4()), + 'queued': 1, + 'skipped': 0, + 'errors': [], + 'run_url': 'https://buildkite.com/organizations/alembic/analytics/suites/test/runs/52c5d9f6-a4f2-4a2d-a1e6-993335789c92'}, + status=202) + + with mock.patch.dict(os.environ, {"CI": "true", "BUILDKITE_ANALYTICS_TOKEN": str(uuid4())}): + payload = Payload.init(detect_env()) + payload = Payload.started(payload) + + payload = payload.push_test_data(successful_test) + payload = payload.push_test_data(failed_test) + + results = [response for response in submit(payload, batch_size=1)] + assert len(results) == 2 + + assert not results[0] + captured = capfd.readouterr() + assert captured.err.startswith("buildkite-test-collector - WARNING -") + assert "ConnectTimeout" in captured.err + + result = results[1] + assert result + assert result.status_code >= 200 + assert result.status_code < 300 + + json = result.json() + assert len(json["errors"]) == 0 + assert json['queued'] == 1 + @responses.activate def test_api_url_override(successful_test): upload_id = str(uuid4())