From a905cce8c8dccbe70237efb38688885100032367 Mon Sep 17 00:00:00 2001 From: Ramin Haeri Azad Date: Wed, 10 Dec 2025 13:59:59 -0500 Subject: [PATCH 01/14] Write binary to stdout buffer --- obiba_mica/rest.py | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/obiba_mica/rest.py b/obiba_mica/rest.py index ebaf458..97c013f 100644 --- a/obiba_mica/rest.py +++ b/obiba_mica/rest.py @@ -103,13 +103,19 @@ def do_command(cls, args): response = request.send() # format response - res = response.as_json() - if args.json: - res = response.pretty_json() - elif args.method in ['OPTIONS']: - res = response.headers['Allow'] - - # output to stdout - print(res) + if args.accept and 'json' not in args.accept.lower(): + # Binary or non-JSON response - output raw content + if response.content: + sys.stdout.buffer.write(response.content) + else: + # JSON response - format and print + res = response.as_json() + if args.json: + res = response.pretty_json() + elif args.method in ['OPTIONS']: + res = response.headers['Allow'] + + # output to stdout + print(res) finally: client.close() From 522cb4efb815cb2bbd9aafcfe93d496a3e47a38d Mon Sep 17 00:00:00 2001 From: Ramin Haeri Azad Date: Wed, 10 Dec 2025 17:45:01 -0500 Subject: [PATCH 02/14] Make tests a little more robust Github --- tests/test_import_zip.py | 3 +- tests/test_updated_collected_datasets.py | 52 ++++++++++++++++-------- 2 files changed, 36 insertions(+), 19 deletions(-) diff --git a/tests/test_import_zip.py b/tests/test_import_zip.py index 96e9dc8..be9db97 100644 --- a/tests/test_import_zip.py +++ b/tests/test_import_zip.py @@ -26,7 +26,8 @@ def __test_changeResourceStatusToDelete(self, restService, resource): def __test_deleteResource(self, restService, resource): try: response = restService.send_request(resource, restService.make_request("DELETE")) - assert response.code == 204, f"Failed to delete resource {resource}: {response.content}" + # Accept 204 (deleted) or 404 (already gone) to make the test idempotent + assert response.code in (204, 404), f"Failed to delete resource {resource}: {response.content}" except Exception as e: assert False, f"Exception while deleting resource {resource}: {e}" diff --git a/tests/test_updated_collected_datasets.py b/tests/test_updated_collected_datasets.py index 696c8c6..1acf59a 100644 --- a/tests/test_updated_collected_datasets.py +++ b/tests/test_updated_collected_datasets.py @@ -1,6 +1,8 @@ import unittest +import time from obiba_mica.update_collected_datasets import CollectedDatasetsService from obiba_mica.legacy import MicaLegacySupport +from obiba_mica.core import HTTPError from tests.utils import Utils class TestClass(unittest.TestCase): @@ -31,26 +33,40 @@ def __updateProjects(self, dataset, project: str): assert False def __unpublish(self, dataset): - try: - response = self.service.unpublish(dataset['id']) - if response.code == 204: - assert True - else: - assert False - - except Exception as e: - assert False + tries = 3 + for attempt in range(tries): + try: + response = self.service.unpublish(dataset['id']) + if response.code == 204: + return + else: + assert False, f"Unexpected response code while unpublishing {dataset['id']}: {response.code}" + except HTTPError as e: + # Retry on server errors (5xx) + if e.is_server_error() and attempt < tries - 1: + time.sleep(2 ** attempt) + continue + assert False, f"HTTPError while unpublishing {dataset['id']}: {e}" + except Exception as e: + assert False, f"Exception while unpublishing {dataset['id']}: {e}" def __publish(self, dataset): - try: - response = self.service.publish(dataset['id']) - if response.code == 204: - assert True - else: - assert False - - except Exception as e: - assert False + tries = 3 + for attempt in range(tries): + try: + response = self.service.publish(dataset['id']) + if response.code == 204: + return + else: + assert False, f"Unexpected response code while publishing {dataset['id']}: {response.code}" + except HTTPError as e: + # Retry on server errors (5xx) + if e.is_server_error() and attempt < tries - 1: + time.sleep(2 ** attempt) + continue + assert False, f"HTTPError while publishing {dataset['id']}: {e}" + except Exception as e: + assert False, f"Exception while publishing {dataset['id']}: {e}" def test_1_getDatasets(self): From 696322e4c50b14f0fa9ca03ab2de320ce71b78c6 Mon Sep 17 00:00:00 2001 From: Ramin Haeri Azad Date: Thu, 11 Dec 2025 07:51:40 -0500 Subject: [PATCH 03/14] Changed to address review comments; added binary dnwld test --- obiba_mica/console.py | 5 ++++- obiba_mica/rest.py | 9 ++++----- tests/test_rest.py | 39 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 6 deletions(-) diff --git a/obiba_mica/console.py b/obiba_mica/console.py index 78ac2e4..c3022f0 100755 --- a/obiba_mica/console.py +++ b/obiba_mica/console.py @@ -113,7 +113,10 @@ def run(): args.func(args) except HTTPError as e: - print(e.error['status'] if e.error is not None else e) + if e.error is not None: + print(e.error.get('status', e.error)) + else: + print(e) sys.exit(2) else: print('Mica command line tool.') diff --git a/obiba_mica/rest.py b/obiba_mica/rest.py index 97c013f..5bf5ad8 100644 --- a/obiba_mica/rest.py +++ b/obiba_mica/rest.py @@ -103,7 +103,10 @@ def do_command(cls, args): response = request.send() # format response - if args.accept and 'json' not in args.accept.lower(): + if args.method in ['OPTIONS']: + # OPTIONS method - extract Allow header + print(response.headers['Allow']) + elif 'json' not in response.headers.get('Content-Type', 'application/json').lower(): # Binary or non-JSON response - output raw content if response.content: sys.stdout.buffer.write(response.content) @@ -112,10 +115,6 @@ def do_command(cls, args): res = response.as_json() if args.json: res = response.pretty_json() - elif args.method in ['OPTIONS']: - res = response.headers['Allow'] - - # output to stdout print(res) finally: client.close() diff --git a/tests/test_rest.py b/tests/test_rest.py index b37c3fb..089b742 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -40,3 +40,42 @@ def test_invalidRestCall(self): assert e.code == 404 except Exception as e: assert False + + def test_binaryDownload(self): + try: + # First, get a list of studies to find a valid study ID + studies_response = self.service.send_request('/draft/individual-studies', self.service.make_request('GET')).as_json() + + if not studies_response or len(studies_response) == 0: + # No studies available, skip test + assert True + return + + # Get the first study's ID + study_id = studies_response[0]['id'] + + # Create request for binary download with Accept: application/octet-stream + request = self.service.make_request('GET') + request.accept('application/octet-stream') + + # Download the study folder as binary (kept in memory, not written to disk) + response = self.service.send_request(f'/draft/file-dl//individual-study/{study_id}', request) + + # Verify we got binary content (not JSON) + assert response.content is not None, "Binary content should not be None" + assert len(response.content) > 0, "Binary content should not be empty" + assert response.code == 200, f"Expected 200, got {response.code}" + + # Verify Content-Type is not JSON (should be application/zip or application/octet-stream) + content_type = response.headers.get('Content-Type', '') + assert 'json' not in content_type.lower(), f"Content-Type should not be JSON, got: {content_type}" + + assert True + except HTTPError as e: + # If the endpoint doesn't exist or study has no files, that's ok for this test + if e.code in [404, 204]: + assert True + else: + assert False, f"Unexpected HTTPError: {e.code}" + except Exception as e: + assert False, f"Unexpected exception: {e}" From 744a7b35394cec7b2b3e5769d098ed08acb45033 Mon Sep 17 00:00:00 2001 From: Ramin Haeri Azad Date: Thu, 11 Dec 2025 08:39:37 -0500 Subject: [PATCH 04/14] Added wait so Github tests pass on the first try! --- tests/test_file.py | 8 ++++ tests/test_import_zip.py | 22 +++++++++++ tests/test_updated_collected_dataset.py | 48 ++++++++++++------------ tests/test_updated_collected_datasets.py | 39 +++++++++---------- tests/utils.py | 48 ++++++++++++++++++++++++ 5 files changed, 122 insertions(+), 43 deletions(-) diff --git a/tests/test_file.py b/tests/test_file.py index a77bfa7..bb903ec 100644 --- a/tests/test_file.py +++ b/tests/test_file.py @@ -14,6 +14,10 @@ def test_1_fileUpload(self): response = self.service.upload('/individual-study', './tests/resources/dummy.csv') if response.code == 201: + # Wait for file to be indexed/available after upload + Utils.wait_for_condition( + lambda: self.service.get('/individual-study/dummy.csv') is not None + ) assert True else: assert False @@ -53,6 +57,10 @@ def test_3_filePublish(self): response = self.service.publish('/individual-study/dummy.csv', True) if response.code == 204: + # Wait for publish to complete/propagate + Utils.wait_for_condition( + lambda: self.service.get('/individual-study/dummy.csv') is not None + ) assert True else: assert False diff --git a/tests/test_import_zip.py b/tests/test_import_zip.py index be9db97..042ac00 100644 --- a/tests/test_import_zip.py +++ b/tests/test_import_zip.py @@ -37,6 +37,17 @@ def test_1_importZip(self): service = FileImportService(self.client) response = service.import_zip("./tests/resources/dummy-test-study.zip", True) assert response.code == 200 + + # Wait for resources to be indexed/available after import + restService = RestService(self.client) + Utils.wait_for_condition( + lambda: restService.send_request("/draft/individual-study/dummy-test-study", + restService.make_request("GET")).code == 200 + ) + Utils.wait_for_condition( + lambda: restService.send_request("/draft/network/dummy-test-network", + restService.make_request("GET")).code == 200 + ) else: assert True except Exception as e: @@ -60,6 +71,17 @@ def test_3_importZip(self): service = FileImportService(self.client) response = service.import_zip("./tests/resources/dummy-test-study-legacy.zip", True, True) assert response.code == 200 + + # Wait for resources to be indexed/available after import + restService = RestService(self.client) + Utils.wait_for_condition( + lambda: restService.send_request("/draft/individual-study/dummy-test-study", + restService.make_request("GET")).code == 200 + ) + Utils.wait_for_condition( + lambda: restService.send_request("/draft/network/dummy-test-network", + restService.make_request("GET")).code == 200 + ) except Exception as e: assert False diff --git a/tests/test_updated_collected_dataset.py b/tests/test_updated_collected_dataset.py index cb24f04..b3a6af6 100644 --- a/tests/test_updated_collected_dataset.py +++ b/tests/test_updated_collected_dataset.py @@ -13,14 +13,14 @@ def test_1_updateProject(self): try: response = self.service.update('cls-wave1', project='dummy') if response.code == 204: - dataset = self.service.get_dataset('cls-wave1') - collectedDataset = MicaLegacySupport.getCollectedDataset(dataset) - studyTable = collectedDataset['studyTable'] - - if studyTable['project'] == 'dummy': - assert True - else: - assert False + # Wait for update to propagate before verifying + def check_update(): + dataset = self.service.get_dataset('cls-wave1') + collectedDataset = MicaLegacySupport.getCollectedDataset(dataset) + studyTable = collectedDataset['studyTable'] + return studyTable.get('project') == 'dummy' + + assert Utils.wait_for_condition(check_update, timeout=5), "Update did not propagate" else: assert False @@ -31,14 +31,14 @@ def test_2_updateTable(self): try: response = self.service.update('cls-wave1', table='dummy') if response.code == 204: - dataset = self.service.get_dataset('cls-wave1') - collectedDataset = MicaLegacySupport.getCollectedDataset(dataset) - studyTable = collectedDataset['studyTable'] - - if studyTable['table'] == 'dummy': - assert True - else: - assert False + # Wait for update to propagate before verifying + def check_update(): + dataset = self.service.get_dataset('cls-wave1') + collectedDataset = MicaLegacySupport.getCollectedDataset(dataset) + studyTable = collectedDataset['studyTable'] + return studyTable.get('table') == 'dummy' + + assert Utils.wait_for_condition(check_update, timeout=5), "Update did not propagate" else: assert False @@ -49,14 +49,14 @@ def test_3_updateProjectTable(self): try: response = self.service.update('cls-wave1', project='CLS', table='Wave1') if response.code == 204: - dataset = self.service.get_dataset('cls-wave1') - collectedDataset = MicaLegacySupport.getCollectedDataset(dataset) - studyTable = collectedDataset['studyTable'] - - if studyTable['project'] == 'CLS' and studyTable['table'] == 'Wave1': - assert True - else: - assert False + # Wait for update to propagate before verifying + def check_update(): + dataset = self.service.get_dataset('cls-wave1') + collectedDataset = MicaLegacySupport.getCollectedDataset(dataset) + studyTable = collectedDataset['studyTable'] + return studyTable.get('project') == 'CLS' and studyTable.get('table') == 'Wave1' + + assert Utils.wait_for_condition(check_update, timeout=5), "Update did not propagate" else: assert False diff --git a/tests/test_updated_collected_datasets.py b/tests/test_updated_collected_datasets.py index 1acf59a..503e43d 100644 --- a/tests/test_updated_collected_datasets.py +++ b/tests/test_updated_collected_datasets.py @@ -1,5 +1,4 @@ import unittest -import time from obiba_mica.update_collected_datasets import CollectedDatasetsService from obiba_mica.legacy import MicaLegacySupport from obiba_mica.core import HTTPError @@ -33,40 +32,42 @@ def __updateProjects(self, dataset, project: str): assert False def __unpublish(self, dataset): - tries = 3 - for attempt in range(tries): + def try_unpublish(): try: response = self.service.unpublish(dataset['id']) if response.code == 204: - return + return True else: + # Unexpected response code, fail immediately assert False, f"Unexpected response code while unpublishing {dataset['id']}: {response.code}" except HTTPError as e: - # Retry on server errors (5xx) - if e.is_server_error() and attempt < tries - 1: - time.sleep(2 ** attempt) - continue + # Retry on server errors (5xx), fail immediately on client errors (4xx) + if e.is_server_error(): + return False assert False, f"HTTPError while unpublishing {dataset['id']}: {e}" - except Exception as e: - assert False, f"Exception while unpublishing {dataset['id']}: {e}" + + # Retry with exponential backoff: 1s, 2s, 4s (max 3 effective tries within ~7s) + success = Utils.wait_for_condition(try_unpublish, timeout=7, interval=1, backoff='exponential') + assert success, f"Failed to unpublish {dataset['id']} after retries" def __publish(self, dataset): - tries = 3 - for attempt in range(tries): + def try_publish(): try: response = self.service.publish(dataset['id']) if response.code == 204: - return + return True else: + # Unexpected response code, fail immediately assert False, f"Unexpected response code while publishing {dataset['id']}: {response.code}" except HTTPError as e: - # Retry on server errors (5xx) - if e.is_server_error() and attempt < tries - 1: - time.sleep(2 ** attempt) - continue + # Retry on server errors (5xx), fail immediately on client errors (4xx) + if e.is_server_error(): + return False assert False, f"HTTPError while publishing {dataset['id']}: {e}" - except Exception as e: - assert False, f"Exception while publishing {dataset['id']}: {e}" + + # Retry with exponential backoff: 1s, 2s, 4s (max 3 effective tries within ~7s) + success = Utils.wait_for_condition(try_publish, timeout=7, interval=1, backoff='exponential') + assert success, f"Failed to publish {dataset['id']} after retries" def test_1_getDatasets(self): diff --git a/tests/utils.py b/tests/utils.py index 233f0b7..0d8de05 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -1,4 +1,5 @@ import sys +import time from argparse import ArgumentParser from obiba_mica import MicaClient @@ -16,6 +17,53 @@ def make_client(server=None, user=None, password=None): password=Utils.PASSWORD if password is None else password, otp=None) + @staticmethod + def wait_for_condition(resource_callback, timeout=10, interval=1, backoff='fixed'): + """ + Poll until a condition is met or timeout occurs. + Useful for CI environments where server processing may be slower after write operations. + + :param resource_callback - callable that returns True when resource is ready, False otherwise + :param timeout - maximum time to wait in seconds (default: 10) + :param interval - base polling interval in seconds (default: 1) + :param backoff - backoff strategy: 'fixed' or 'exponential' (default: 'fixed') + For exponential: sleeps interval * 2^attempt (1s, 2s, 4s, 8s, ...) + :return True if condition met, False if timeout + + Example usage (fixed interval): + Utils.wait_for_condition( + lambda: restService.send_request('/draft/study/id', restService.make_request('GET')).code == 200 + ) + + Example usage (exponential backoff for retries): + Utils.wait_for_condition( + lambda: service.publish(id).code == 204, + timeout=10, + interval=1, + backoff='exponential' + ) + """ + elapsed = 0 + attempt = 0 + while elapsed < timeout: + try: + if resource_callback(): + return True + except Exception: + # Condition not met yet, keep waiting + pass + + # Calculate sleep time based on backoff strategy + if backoff == 'exponential': + sleep_time = interval * (2 ** attempt) + else: + sleep_time = interval + + time.sleep(sleep_time) + elapsed += sleep_time + attempt += 1 + return False + @staticmethod def make_arg_parser(): parser = ArgumentParser() From 208b80e78644fcdce4760a8768d8339246c0cf94 Mon Sep 17 00:00:00 2001 From: Ramin Haeri Azad Date: Thu, 11 Dec 2025 08:52:28 -0500 Subject: [PATCH 05/14] Added do not fail on error when applicable --- obiba_mica/core.py | 4 ++++ tests/test_import_zip.py | 4 +++- tests/test_updated_collected_dataset.py | 6 +++--- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/obiba_mica/core.py b/obiba_mica/core.py index a8f3262..c43f224 100755 --- a/obiba_mica/core.py +++ b/obiba_mica/core.py @@ -190,6 +190,10 @@ def fail_on_error(self): self._fail_on_error = True return self + def ignore_fail_on_error(self): + self._fail_on_error = False + return self + def header(self, key, value): """ Adds a header to session headers used by the request diff --git a/tests/test_import_zip.py b/tests/test_import_zip.py index 042ac00..c969803 100644 --- a/tests/test_import_zip.py +++ b/tests/test_import_zip.py @@ -25,7 +25,9 @@ def __test_changeResourceStatusToDelete(self, restService, resource): def __test_deleteResource(self, restService, resource): try: - response = restService.send_request(resource, restService.make_request("DELETE")) + # Don't fail on errors so we can handle 404 as valid (idempotent delete) + request = restService.make_request("DELETE").ignore_fail_on_error() + response = restService.send_request(resource, request) # Accept 204 (deleted) or 404 (already gone) to make the test idempotent assert response.code in (204, 404), f"Failed to delete resource {resource}: {response.content}" except Exception as e: diff --git a/tests/test_updated_collected_dataset.py b/tests/test_updated_collected_dataset.py index b3a6af6..caaeb27 100644 --- a/tests/test_updated_collected_dataset.py +++ b/tests/test_updated_collected_dataset.py @@ -20,7 +20,7 @@ def check_update(): studyTable = collectedDataset['studyTable'] return studyTable.get('project') == 'dummy' - assert Utils.wait_for_condition(check_update, timeout=5), "Update did not propagate" + assert Utils.wait_for_condition(check_update, timeout=10), "Update did not propagate" else: assert False @@ -38,7 +38,7 @@ def check_update(): studyTable = collectedDataset['studyTable'] return studyTable.get('table') == 'dummy' - assert Utils.wait_for_condition(check_update, timeout=5), "Update did not propagate" + assert Utils.wait_for_condition(check_update, timeout=10), "Update did not propagate" else: assert False @@ -56,7 +56,7 @@ def check_update(): studyTable = collectedDataset['studyTable'] return studyTable.get('project') == 'CLS' and studyTable.get('table') == 'Wave1' - assert Utils.wait_for_condition(check_update, timeout=5), "Update did not propagate" + assert Utils.wait_for_condition(check_update, timeout=10), "Update did not propagate" else: assert False From 8ff82ca81f3b7c593a354e472c72510f2faa6696 Mon Sep 17 00:00:00 2001 From: Ramin Haeri Azad Date: Thu, 11 Dec 2025 10:08:20 -0500 Subject: [PATCH 06/14] Increased wait time when tests are run in Github actions --- tests/test_access.py | 20 ++++++++++++-------- tests/test_import_zip.py | 22 ++++++++++++++-------- tests/test_permission.py | 10 ++++++---- tests/test_updated_collected_dataset.py | 6 +++--- tests/test_updated_collected_datasets.py | 10 ++++++---- tests/utils.py | 20 ++++++++++++++++++++ 6 files changed, 61 insertions(+), 27 deletions(-) diff --git a/tests/test_access.py b/tests/test_access.py index 6b1f31a..d461de8 100644 --- a/tests/test_access.py +++ b/tests/test_access.py @@ -11,11 +11,13 @@ def test_documentAccess(self): response = self.service.add_access('clsa', 'USER', 'user1') assert response.code == 204 - response = self.service.list_accesses('clsa').as_json() - found = next((x for x in response if x['principal'] == 'user1'), None) + # Wait for access to be indexed/available + def check_access(): + response = self.service.list_accesses('clsa').as_json() + found = next((x for x in response if x['principal'] == 'user1'), None) + return found is not None - if found is None: - assert False + assert Utils.wait_for_condition(check_access, timeout=Utils.get_timeout(10)), "Access not found after add" response = self.service.delete_access('clsa', 'USER', 'user1') assert response.code == 204 @@ -30,11 +32,13 @@ def test_fileAccess(self): response = self.service.add_access(file, 'USER', 'user1') assert response.code == 204 - response = self.service.list_accesses(file).as_json() - found = next((x for x in response if x['principal'] == 'user1'), None) + # Wait for access to be indexed/available + def check_access(): + response = self.service.list_accesses(file).as_json() + found = next((x for x in response if x['principal'] == 'user1'), None) + return found is not None - if found is None: - assert False + assert Utils.wait_for_condition(check_access, timeout=Utils.get_timeout(10)), "File access not found after add" response = self.service.delete_access(file, 'USER', 'user1') assert response.code == 204 diff --git a/tests/test_import_zip.py b/tests/test_import_zip.py index c969803..fd01a86 100644 --- a/tests/test_import_zip.py +++ b/tests/test_import_zip.py @@ -12,16 +12,22 @@ def setup_class(cls): cls.needsLegacySupport = FileImportService.needsLegacySupport(cls.client) def __test_changeResourceStatusToDelete(self, restService, resource): - try: - response = restService.send_request("%s/_status?value=DELETED" % resource, restService.make_request("PUT")) + from obiba_mica.core import HTTPError - if response.code == 204: - assert True - else: - assert False + def try_status_change(): + try: + response = restService.send_request("%s/_status?value=DELETED" % resource, restService.make_request("PUT")) + return response.code == 204 + except HTTPError as e: + # Retry on server errors (5xx) - resource might not be ready yet + if e.is_server_error(): + return False + raise - except Exception as e: - assert False + # Retry with exponential backoff - longer timeout in CI + timeout = Utils.get_timeout(7) # 7s local, 21s in CI + success = Utils.wait_for_condition(try_status_change, timeout=timeout, interval=1, backoff='exponential') + assert success, f"Failed to change status to DELETED for {resource}" def __test_deleteResource(self, restService, resource): try: diff --git a/tests/test_permission.py b/tests/test_permission.py index 5f218da..1f2bc2e 100644 --- a/tests/test_permission.py +++ b/tests/test_permission.py @@ -13,11 +13,13 @@ def test_documentPermission(self): response = self.service.add_permission('clsa', 'USER', 'user1', 'READER') assert response.code == 204 - response = self.service.list_permissions('clsa').as_json() - found = next((x for x in response if x['principal'] == 'user1'), None) + # Wait for permission to be indexed/available + def check_permission(): + response = self.service.list_permissions('clsa').as_json() + found = next((x for x in response if x['principal'] == 'user1'), None) + return found is not None - if found is None: - assert False + assert Utils.wait_for_condition(check_permission, timeout=Utils.get_timeout(10)), "Permission not found after add" response = self.service.delete_permission('clsa', 'USER', 'user1') assert response.code == 204 diff --git a/tests/test_updated_collected_dataset.py b/tests/test_updated_collected_dataset.py index caaeb27..af6a906 100644 --- a/tests/test_updated_collected_dataset.py +++ b/tests/test_updated_collected_dataset.py @@ -20,7 +20,7 @@ def check_update(): studyTable = collectedDataset['studyTable'] return studyTable.get('project') == 'dummy' - assert Utils.wait_for_condition(check_update, timeout=10), "Update did not propagate" + assert Utils.wait_for_condition(check_update, timeout=Utils.get_timeout(10)), "Update did not propagate" else: assert False @@ -38,7 +38,7 @@ def check_update(): studyTable = collectedDataset['studyTable'] return studyTable.get('table') == 'dummy' - assert Utils.wait_for_condition(check_update, timeout=10), "Update did not propagate" + assert Utils.wait_for_condition(check_update, timeout=Utils.get_timeout(10)), "Update did not propagate" else: assert False @@ -56,7 +56,7 @@ def check_update(): studyTable = collectedDataset['studyTable'] return studyTable.get('project') == 'CLS' and studyTable.get('table') == 'Wave1' - assert Utils.wait_for_condition(check_update, timeout=10), "Update did not propagate" + assert Utils.wait_for_condition(check_update, timeout=Utils.get_timeout(10)), "Update did not propagate" else: assert False diff --git a/tests/test_updated_collected_datasets.py b/tests/test_updated_collected_datasets.py index 503e43d..984c299 100644 --- a/tests/test_updated_collected_datasets.py +++ b/tests/test_updated_collected_datasets.py @@ -46,8 +46,9 @@ def try_unpublish(): return False assert False, f"HTTPError while unpublishing {dataset['id']}: {e}" - # Retry with exponential backoff: 1s, 2s, 4s (max 3 effective tries within ~7s) - success = Utils.wait_for_condition(try_unpublish, timeout=7, interval=1, backoff='exponential') + # Retry with exponential backoff: 1s, 2s, 4s - longer timeout in CI + timeout = Utils.get_timeout(7) # 7s local, 21s in CI + success = Utils.wait_for_condition(try_unpublish, timeout=timeout, interval=1, backoff='exponential') assert success, f"Failed to unpublish {dataset['id']} after retries" def __publish(self, dataset): @@ -65,8 +66,9 @@ def try_publish(): return False assert False, f"HTTPError while publishing {dataset['id']}: {e}" - # Retry with exponential backoff: 1s, 2s, 4s (max 3 effective tries within ~7s) - success = Utils.wait_for_condition(try_publish, timeout=7, interval=1, backoff='exponential') + # Retry with exponential backoff: 1s, 2s, 4s - longer timeout in CI + timeout = Utils.get_timeout(7) # 7s local, 21s in CI + success = Utils.wait_for_condition(try_publish, timeout=timeout, interval=1, backoff='exponential') assert success, f"Failed to publish {dataset['id']} after retries" diff --git a/tests/utils.py b/tests/utils.py index 0d8de05..d000b71 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -1,5 +1,6 @@ import sys import time +import os from argparse import ArgumentParser from obiba_mica import MicaClient @@ -10,6 +11,25 @@ class Utils: PASSWORD = 'password' DEFAULT_PARAMS = {"accept": False, "content_type": False, "verbose": False, "method": 'GET'} + @staticmethod + def is_ci_environment(): + """ + Detect if running in CI environment (GitHub Actions, etc.) + Returns True if CI environment variables are set + """ + return os.getenv('CI', '').lower() == 'true' or os.getenv('GITHUB_ACTIONS', '').lower() == 'true' + + @staticmethod + def get_timeout(base_timeout): + """ + Get timeout adjusted for CI environment. + CI environments are slower, so use 3x the base timeout. + + :param base_timeout - base timeout in seconds for local development + :return adjusted timeout (base * 3 if CI, else base) + """ + return base_timeout * 3 if Utils.is_ci_environment() else base_timeout + @staticmethod def make_client(server=None, user=None, password=None): return MicaClient.buildWithAuthentication(server= Utils.SERVER if server is None else server, From 51d649c1fcd4837212c43e4c7f4da3d580053582 Mon Sep 17 00:00:00 2001 From: Ramin Haeri Azad Date: Thu, 11 Dec 2025 10:11:56 -0500 Subject: [PATCH 07/14] Fixed test_file --- tests/test_file.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/test_file.py b/tests/test_file.py index bb903ec..ace53a1 100644 --- a/tests/test_file.py +++ b/tests/test_file.py @@ -16,7 +16,8 @@ def test_1_fileUpload(self): if response.code == 201: # Wait for file to be indexed/available after upload Utils.wait_for_condition( - lambda: self.service.get('/individual-study/dummy.csv') is not None + lambda: self.service.get('/individual-study/dummy.csv') is not None, + timeout=Utils.get_timeout(10) ) assert True else: @@ -59,7 +60,8 @@ def test_3_filePublish(self): if response.code == 204: # Wait for publish to complete/propagate Utils.wait_for_condition( - lambda: self.service.get('/individual-study/dummy.csv') is not None + lambda: self.service.get('/individual-study/dummy.csv') is not None, + timeout=Utils.get_timeout(10) ) assert True else: From b1c22ca0beafe1fc8ef120dc83e7abc26162284b Mon Sep 17 00:00:00 2001 From: Ramin Haeri Azad Date: Thu, 11 Dec 2025 10:22:12 -0500 Subject: [PATCH 08/14] Fixed test_import --- tests/test_import_zip.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/test_import_zip.py b/tests/test_import_zip.py index fd01a86..836f17e 100644 --- a/tests/test_import_zip.py +++ b/tests/test_import_zip.py @@ -50,11 +50,13 @@ def test_1_importZip(self): restService = RestService(self.client) Utils.wait_for_condition( lambda: restService.send_request("/draft/individual-study/dummy-test-study", - restService.make_request("GET")).code == 200 + restService.make_request("GET")).code == 200, + timeout=Utils.get_timeout(10) ) Utils.wait_for_condition( lambda: restService.send_request("/draft/network/dummy-test-network", - restService.make_request("GET")).code == 200 + restService.make_request("GET")).code == 200, + timeout=Utils.get_timeout(10) ) else: assert True @@ -84,11 +86,13 @@ def test_3_importZip(self): restService = RestService(self.client) Utils.wait_for_condition( lambda: restService.send_request("/draft/individual-study/dummy-test-study", - restService.make_request("GET")).code == 200 + restService.make_request("GET")).code == 200, + timeout=Utils.get_timeout(10) ) Utils.wait_for_condition( lambda: restService.send_request("/draft/network/dummy-test-network", - restService.make_request("GET")).code == 200 + restService.make_request("GET")).code == 200, + timeout=Utils.get_timeout(10) ) except Exception as e: assert False From 0b8477d8f08fa21f9298b99d5239f991ff0afc94 Mon Sep 17 00:00:00 2001 From: Ramin Haeri Azad Date: Thu, 11 Dec 2025 10:36:39 -0500 Subject: [PATCH 09/14] Fixed race condition for 409 --- tests/test_file.py | 23 +++++++++++++++++++++++ tests/test_import_zip.py | 30 ++++++++++++++++++++++-------- 2 files changed, 45 insertions(+), 8 deletions(-) diff --git a/tests/test_file.py b/tests/test_file.py index ace53a1..4cedf04 100644 --- a/tests/test_file.py +++ b/tests/test_file.py @@ -99,6 +99,17 @@ def test_5_fileDownload(self): def test_6_changeDeletedStatus(self): self.__test_fileChangeStatus('/individual-study/dummy.csv', FileService.STATUS_DELETED) + # Wait for status change to propagate before test_7 tries to delete + def check_status(): + try: + state = self.service.get('/individual-study/dummy.csv').as_json() + return state.get('revisionStatus') == FileService.STATUS_DELETED + except Exception: + return False + + assert Utils.wait_for_condition(check_status, timeout=Utils.get_timeout(10)), \ + "File status did not propagate to DELETED" + def test_7_fileDelete(self): self.__test_fileDelete('/individual-study/dummy.csv') @@ -108,6 +119,18 @@ def test_8_createFolder(self): if response.code == 201: self.__test_fileChangeStatus('/individual-study/yoyo', FileService.STATUS_DELETED) + + # Wait for status change to propagate before delete + def check_status(): + try: + state = self.service.get('/individual-study/yoyo').as_json() + return state.get('revisionStatus') == FileService.STATUS_DELETED + except Exception: + return False + + assert Utils.wait_for_condition(check_status, timeout=Utils.get_timeout(10)), \ + "Folder status did not propagate to DELETED" + self.__test_fileDelete('/individual-study/yoyo') else: diff --git a/tests/test_import_zip.py b/tests/test_import_zip.py index 836f17e..3a5008b 100644 --- a/tests/test_import_zip.py +++ b/tests/test_import_zip.py @@ -30,14 +30,28 @@ def try_status_change(): assert success, f"Failed to change status to DELETED for {resource}" def __test_deleteResource(self, restService, resource): - try: - # Don't fail on errors so we can handle 404 as valid (idempotent delete) - request = restService.make_request("DELETE").ignore_fail_on_error() - response = restService.send_request(resource, request) - # Accept 204 (deleted) or 404 (already gone) to make the test idempotent - assert response.code in (204, 404), f"Failed to delete resource {resource}: {response.content}" - except Exception as e: - assert False, f"Exception while deleting resource {resource}: {e}" + def try_delete(): + try: + # Don't fail on errors so we can handle 404 and 409 as retriable + request = restService.make_request("DELETE").ignore_fail_on_error() + response = restService.send_request(resource, request) + + # 204 = deleted, 404 = already gone (both success) + if response.code in (204, 404): + return True + # 409 = conflict (still has dependencies, retry) + elif response.code == 409: + return False + else: + # Unexpected error code, fail immediately + assert False, f"Unexpected response {response.code} deleting {resource}: {response.content}" + except Exception as e: + assert False, f"Exception while deleting resource {resource}: {e}" + + # Retry delete with exponential backoff for 409 conflicts + timeout = Utils.get_timeout(15) # 15s local, 45s in CI + success = Utils.wait_for_condition(try_delete, timeout=timeout, interval=1, backoff='exponential') + assert success, f"Failed to delete resource {resource} after {timeout}s (dependencies not cleared)" def test_1_importZip(self): try: From f85c0b850c38481603c1a2d58be46352383481b6 Mon Sep 17 00:00:00 2001 From: Ramin Haeri Azad Date: Thu, 11 Dec 2025 10:47:18 -0500 Subject: [PATCH 10/14] Fixed race condition --- tests/test_file.py | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/tests/test_file.py b/tests/test_file.py index 4cedf04..0544f44 100644 --- a/tests/test_file.py +++ b/tests/test_file.py @@ -27,16 +27,22 @@ def test_1_fileUpload(self): assert False def __test_fileChangeStatus(self, file, status): - try: - response = self.service.status(file, status) + from obiba_mica.core import HTTPError - if response.code == 204: - assert True - else: - assert False - - except Exception as e: - assert False + def try_status_change(): + try: + response = self.service.status(file, status) + return response.code == 204 + except HTTPError as e: + # Retry on 404 (file not indexed yet) or 5xx (server errors) + if e.code == 404 or e.is_server_error(): + return False + raise + + # Retry with exponential backoff - longer timeout in CI + timeout = Utils.get_timeout(7) # 7s local, 21s in CI + success = Utils.wait_for_condition(try_status_change, timeout=timeout, interval=1, backoff='exponential') + assert success, f"Failed to change status to {status} for {file}" def __test_fileDelete(self, path): try: From 41ea26b7068f7fbea8592b80574962533505074d Mon Sep 17 00:00:00 2001 From: Ramin Haeri Azad Date: Thu, 11 Dec 2025 10:54:47 -0500 Subject: [PATCH 11/14] Fixed race condition --- obiba_mica/file.py | 25 ++++++++++++++++++++----- tests/test_import_zip.py | 4 ++-- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/obiba_mica/file.py b/obiba_mica/file.py index 52425a7..4e59075 100755 --- a/obiba_mica/file.py +++ b/obiba_mica/file.py @@ -4,12 +4,13 @@ import argparse import json -from obiba_mica.core import MicaClient +from obiba_mica.core import MicaClient, HTTPError import urllib.request import urllib.parse import urllib.error import re import os +import time class FileAction(argparse.Action): """ @@ -83,11 +84,25 @@ def __make_request(self): def __validate_status(self, file, status): """ Validates the input request, must match the actual file status + Retries on 404 errors with exponential backoff to handle eventual consistency """ - state = self.get(file).as_json() - if state['revisionStatus'] != status: - raise Exception('Invalid file revision status. Found: %s, Required: %s' % ( - state['revisionStatus'], status)) + max_retries = 7 + retry_delay = 1 + + for attempt in range(max_retries): + try: + state = self.get(file).as_json() + if state['revisionStatus'] != status: + raise Exception('Invalid file revision status. Found: %s, Required: %s' % ( + state['revisionStatus'], status)) + return # Success + except HTTPError as e: + if e.code == 404 and attempt < max_retries - 1: + # File not available yet (eventual consistency), retry with exponential backoff + time.sleep(retry_delay) + retry_delay *= 2 + else: + raise # Re-raise if not 404 or out of retries def get(self, file): """ diff --git a/tests/test_import_zip.py b/tests/test_import_zip.py index 3a5008b..43d9e1d 100644 --- a/tests/test_import_zip.py +++ b/tests/test_import_zip.py @@ -19,8 +19,8 @@ def try_status_change(): response = restService.send_request("%s/_status?value=DELETED" % resource, restService.make_request("PUT")) return response.code == 204 except HTTPError as e: - # Retry on server errors (5xx) - resource might not be ready yet - if e.is_server_error(): + # Retry on 404 (resource not indexed yet) or 5xx (server errors) + if e.code == 404 or e.is_server_error(): return False raise From c5fcd38a9ae68e45f1a9b90aa4096dc1904ba7c6 Mon Sep 17 00:00:00 2001 From: Ramin Haeri Azad Date: Thu, 11 Dec 2025 11:00:18 -0500 Subject: [PATCH 12/14] Fixed race condition --- tests/test_import_zip.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/test_import_zip.py b/tests/test_import_zip.py index 43d9e1d..c3946e3 100644 --- a/tests/test_import_zip.py +++ b/tests/test_import_zip.py @@ -14,20 +14,29 @@ def setup_class(cls): def __test_changeResourceStatusToDelete(self, restService, resource): from obiba_mica.core import HTTPError + last_error = None + def try_status_change(): + nonlocal last_error try: response = restService.send_request("%s/_status?value=DELETED" % resource, restService.make_request("PUT")) return response.code == 204 except HTTPError as e: + last_error = e # Retry on 404 (resource not indexed yet) or 5xx (server errors) if e.code == 404 or e.is_server_error(): return False raise # Retry with exponential backoff - longer timeout in CI - timeout = Utils.get_timeout(7) # 7s local, 21s in CI + # Using longer timeout since status changes can take time after import + timeout = Utils.get_timeout(10) # 10s local, 30s in CI success = Utils.wait_for_condition(try_status_change, timeout=timeout, interval=1, backoff='exponential') - assert success, f"Failed to change status to DELETED for {resource}" + if not success: + error_msg = f"Failed to change status to DELETED for {resource}" + if last_error: + error_msg += f" - Last error: {last_error.code} {last_error}" + assert False, error_msg def __test_deleteResource(self, restService, resource): def try_delete(): From 928c63f585806e0d1a7f2326803006235c53d92f Mon Sep 17 00:00:00 2001 From: Ramin Haeri Azad Date: Thu, 11 Dec 2025 11:17:08 -0500 Subject: [PATCH 13/14] Fixed race condition --- tests/test_access.py | 17 +++++++++++ tests/test_file.py | 27 ++++++++++++++++++ tests/test_import_zip.py | 36 ++++++++++++++++++++++++ tests/test_permission.py | 13 +++++++++ tests/test_updated_collected_dataset.py | 21 ++++++++++++++ tests/test_updated_collected_datasets.py | 26 +++++++++++++++++ 6 files changed, 140 insertions(+) diff --git a/tests/test_access.py b/tests/test_access.py index d461de8..860ab91 100644 --- a/tests/test_access.py +++ b/tests/test_access.py @@ -4,6 +4,23 @@ class TestClass(unittest.TestCase): + def setUp(self): + """Clean up before each test to ensure test isolation""" + # Clean up document access + try: + service = IndividualStudyAccessService(Utils.make_client()) + service.delete_access('clsa', 'USER', 'user1') + except Exception: + pass + + # Clean up file access + try: + service = FileAccessService(Utils.make_client()) + file = '/individual-study/cls/population/1/data-collection-event/4/Wave 4 subject interview.pdf' + service.delete_access(file, 'USER', 'user1') + except Exception: + pass + def test_documentAccess(self): self.service = IndividualStudyAccessService(Utils.make_client()) diff --git a/tests/test_file.py b/tests/test_file.py index 0544f44..0c47aae 100644 --- a/tests/test_file.py +++ b/tests/test_file.py @@ -8,6 +8,33 @@ class TestClass(unittest.TestCase): @classmethod def setup_class(cls): cls.service = FileService(Utils.make_client()) + # Clean up any leftover file from previous test runs + cls._cleanup_test_file() + + @classmethod + def teardown_class(cls): + # Clean up after all tests complete + cls._cleanup_test_file() + + @classmethod + def _cleanup_test_file(cls): + """Clean up test file to ensure test isolation""" + from obiba_mica.core import HTTPError + try: + existing = cls.service.get('/individual-study/dummy.csv') + if existing: + current_status = existing.as_json().get('revisionStatus') + if current_status != FileService.STATUS_DELETED: + try: + cls.service.status('/individual-study/dummy.csv', FileService.STATUS_DELETED) + except Exception: + pass + try: + cls.service.delete('/individual-study/dummy.csv') + except Exception: + pass + except HTTPError: + pass # File doesn't exist, which is fine def test_1_fileUpload(self): try: diff --git a/tests/test_import_zip.py b/tests/test_import_zip.py index c3946e3..b700ef2 100644 --- a/tests/test_import_zip.py +++ b/tests/test_import_zip.py @@ -10,6 +10,42 @@ class TestClass(unittest.TestCase): def setup_class(cls): cls.client = Utils.make_client() cls.needsLegacySupport = FileImportService.needsLegacySupport(cls.client) + # Clean up any leftover resources from previous test runs + cls._cleanup_test_resources() + + @classmethod + def teardown_class(cls): + # Clean up after all tests complete + cls._cleanup_test_resources() + + @classmethod + def _cleanup_test_resources(cls): + """Clean up test resources to ensure test isolation""" + from obiba_mica.core import HTTPError + restService = RestService(cls.client) + + resources = [ + "/draft/network/dummy-test-network", + "/draft/individual-study/dummy-test-study" + ] + + for resource in resources: + try: + # Try to change status to DELETED + try: + restService.send_request(f"{resource}/_status?value=DELETED", + restService.make_request("PUT")) + except Exception: + pass + + # Try to delete + try: + request = restService.make_request("DELETE").ignore_fail_on_error() + restService.send_request(resource, request) + except Exception: + pass + except Exception: + pass # Ignore all cleanup errors def __test_changeResourceStatusToDelete(self, restService, resource): from obiba_mica.core import HTTPError diff --git a/tests/test_permission.py b/tests/test_permission.py index 1f2bc2e..2ddcaa4 100644 --- a/tests/test_permission.py +++ b/tests/test_permission.py @@ -7,6 +7,19 @@ class TestClass(unittest.TestCase): @classmethod def setup_class(cls): cls.service = IndividualStudyPermissionService(Utils.make_client()) + # Clean up any leftover permissions from previous test runs + try: + cls.service.delete_permission('clsa', 'USER', 'user1') + except Exception: + pass + + @classmethod + def teardown_class(cls): + # Clean up after all tests complete + try: + cls.service.delete_permission('clsa', 'USER', 'user1') + except Exception: + pass def test_documentPermission(self): try: diff --git a/tests/test_updated_collected_dataset.py b/tests/test_updated_collected_dataset.py index af6a906..10dd219 100644 --- a/tests/test_updated_collected_dataset.py +++ b/tests/test_updated_collected_dataset.py @@ -8,6 +8,27 @@ class TestClass(unittest.TestCase): @classmethod def setup_class(cls): cls.service = CollectedDatasetService(Utils.make_client()) + # Ensure dataset is in expected state before tests + cls._ensure_dataset_state() + + @classmethod + def teardown_class(cls): + # Restore dataset to expected state after tests + cls._ensure_dataset_state() + + @classmethod + def _ensure_dataset_state(cls): + """Ensure cls-wave1 dataset is in the correct state (published, correct project/table)""" + try: + # Restore correct project and table values + cls.service.update('cls-wave1', project='CLS', table='Wave1') + # Ensure it's published + try: + cls.service.publish('cls-wave1') + except Exception: + pass # May already be published + except Exception: + pass # Dataset may not exist, which is fine for some test environments def test_1_updateProject(self): try: diff --git a/tests/test_updated_collected_datasets.py b/tests/test_updated_collected_datasets.py index 984c299..2b69c6b 100644 --- a/tests/test_updated_collected_datasets.py +++ b/tests/test_updated_collected_datasets.py @@ -10,6 +10,32 @@ class TestClass(unittest.TestCase): @classmethod def setup_class(cls): cls.service = CollectedDatasetsService(Utils.make_client()) + # Ensure datasets are in expected state before tests + cls._ensure_datasets_state() + + @classmethod + def teardown_class(cls): + # Restore datasets to expected state after tests + cls._ensure_datasets_state() + + @classmethod + def _ensure_datasets_state(cls): + """Ensure cls datasets are in the correct state (published, correct project)""" + try: + datasets = cls.service.get_datasets('^cls-') + for dataset in datasets: + try: + # Restore correct project value + cls.service.update(dataset['id'], 'CLS') + # Ensure it's published + try: + cls.service.publish(dataset['id']) + except Exception: + pass # May already be published + except Exception: + pass # Individual dataset cleanup failure + except Exception: + pass # Datasets may not exist, which is fine for some test environments def __updateProjects(self, dataset, project: str): From 00ec66c0b22e4d84bedd00fb888684f0ed67206d Mon Sep 17 00:00:00 2001 From: Ramin Haeri Azad Date: Thu, 11 Dec 2025 11:28:58 -0500 Subject: [PATCH 14/14] Run test sequentially --- .github/workflows/ci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 981ad6d..dec7506 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -10,6 +10,7 @@ jobs: test: runs-on: ubuntu-latest strategy: + max-parallel: 1 # Run tests sequentially to avoid conflicts on shared mica-demo.obiba.org server matrix: python-version: [3.8.18, 3.10.18, 3.12.11] steps: