From 210cbcf9f9911b2e62f6c5f80083982cc945d8e4 Mon Sep 17 00:00:00 2001 From: Simranpal Date: Wed, 12 Nov 2025 14:10:04 -0800 Subject: [PATCH] Fix job type check in some methods & improve related unit tests --- src/objects/job.py | 17 ++++- ...firewatch_objects_job_get_all_build_ids.py | 74 ++++++++++++++++--- ...est_firewatch_objects_job_get_timestamp.py | 63 ++++++++++++++-- 3 files changed, 136 insertions(+), 18 deletions(-) diff --git a/src/objects/job.py b/src/objects/job.py index 216af10..eb0f4e8 100644 --- a/src/objects/job.py +++ b/src/objects/job.py @@ -312,11 +312,14 @@ def _get_steps( steps = [] + if self.is_rehearsal: + prefix = f"pr-logs/pull/openshift_release/{self.pr_id}/{job_name}/{build_id}/artifacts/{job_name_safe}" + else: + prefix = f"logs/{job_name}/{build_id}/artifacts/{job_name_safe}" blobs = storage_client.list_blobs( gcs_bucket, - prefix=f"logs/{job_name}/{build_id}/artifacts/{job_name_safe}", + prefix=prefix, ) - # Populate list of steps for blob in blobs: blob_step = blob.name.split("/")[-2] @@ -523,7 +526,10 @@ def _get_timestamp( try: # Construct the blob path bucket = storage_client.bucket(gcs_bucket) - blob_path = f"logs/{job_name}/{build_id}/started.json" + if self.is_rehearsal: + blob_path = f"pr-logs/pull/openshift_release/{self.pr_id}/{job_name}/{build_id}/started.json" + else: + blob_path = f"logs/{job_name}/{build_id}/started.json" blob = bucket.blob(blob_path) # Get the timestamp from path @@ -556,7 +562,10 @@ def _get_all_build_ids( try: bucket = storage_client.bucket(gcs_bucket) - prefix = f"logs/{job_name}/" + if self.is_rehearsal: + prefix = f"pr-logs/pull/openshift_release/{self.pr_id}/{job_name}/" + else: + prefix = f"logs/{job_name}/" blobs = bucket.list_blobs(prefix=prefix, delimiter="/") build_ids = [] diff --git a/tests/unittests/objects/job/test_firewatch_objects_job_get_all_build_ids.py b/tests/unittests/objects/job/test_firewatch_objects_job_get_all_build_ids.py index af99158..e9d4856 100644 --- a/tests/unittests/objects/job/test_firewatch_objects_job_get_all_build_ids.py +++ b/tests/unittests/objects/job/test_firewatch_objects_job_get_all_build_ids.py @@ -6,8 +6,11 @@ class TestGetAllBuildIds(JobBaseTest): @patch("src.objects.job.storage.Client") - def test_get_all_build_ids(self, mock_storage_client): - # Mock the storage client and bucket + def test_get_all_build_ids_rehearsal_job(self, mock_storage_client): + """ + Tests that _get_all_build_ids uses the 'pr-logs' prefix for rehearsal jobs. + """ + # --- Setup Mocks --- mock_client = MagicMock() mock_bucket = MagicMock() mock_blobs = MagicMock() @@ -16,30 +19,83 @@ def test_get_all_build_ids(self, mock_storage_client): mock_client.bucket.return_value = mock_bucket mock_bucket.list_blobs.return_value = mock_blobs - # Mock the list of blobs with prefixes + # Mock the list of *correct* prefixes for a rehearsal job mock_blobs.pages = [ MagicMock( prefixes=[ - "logs/rehearse-1234-job1/8123/", - "logs/rehearse-1234-job1/8124/", - "logs/rehearse-1234-job1/8125/", + "pr-logs/pull/openshift_release/1234/rehearse-1234-job1/8123/", + "pr-logs/pull/openshift_release/1234/rehearse-1234-job1/8124/", ] ) ] + # --- Create Rehearsal Job --- job = Job( name="rehearse-1234-job1", name_safe="job1_safe", - build_id="8123", + build_id="8124", gcs_bucket="bucket1", gcs_creds_file=None, firewatch_config=self.config, ) + # --- Run Method --- build_ids = job._get_all_build_ids( - job_name="rehearse-1234-job1", storage_client=mock_client, gcs_bucket=mock_bucket + job_name="rehearse-1234-job1", storage_client=mock_client, gcs_bucket="bucket1" ) - assert build_ids == ["8123", "8124", "8125"] + + # --- Assertions --- + # 1. Assert the list is correct + assert build_ids == ["8123", "8124"] + + # 2. (CRITICAL) Assert the correct prefix was used in the GCS call + expected_prefix = "pr-logs/pull/openshift_release/1234/rehearse-1234-job1/" + mock_bucket.list_blobs.assert_called_with(prefix=expected_prefix, delimiter="/") + + @patch("src.objects.job.storage.Client") + def test_get_all_build_ids_regular_job(self, mock_storage_client): + """ + Tests that _get_all_build_ids uses the 'logs' prefix for regular jobs. + """ + # --- Setup Mocks --- + mock_client = MagicMock() + mock_bucket = MagicMock() + mock_blobs = MagicMock() + + mock_storage_client.return_value = mock_client + mock_client.bucket.return_value = mock_bucket + mock_bucket.list_blobs.return_value = mock_blobs + + # Mock the list of prefixes for a regular job + mock_blobs.pages = [ + MagicMock( + prefixes=[ + "logs/periodic-job-1/100/", + "logs/periodic-job-1/101/", + ] + ) + ] + + # --- Create Regular Job --- + job = Job( + name="periodic-job-1", + name_safe="job1_safe", + build_id="101", + gcs_bucket="bucket1", + gcs_creds_file=None, + firewatch_config=self.config, + ) + + # --- Run Method --- + build_ids = job._get_all_build_ids(job_name="periodic-job-1", storage_client=mock_client, gcs_bucket="bucket1") + + # --- Assertions --- + # 1. Assert the list is correct + assert build_ids == ["100", "101"] + + # 2. (CRITICAL) Assert the correct prefix was used in the GCS call + expected_prefix = "logs/periodic-job-1/" + mock_bucket.list_blobs.assert_called_with(prefix=expected_prefix, delimiter="/") if __name__ == "__main__": diff --git a/tests/unittests/objects/job/test_firewatch_objects_job_get_timestamp.py b/tests/unittests/objects/job/test_firewatch_objects_job_get_timestamp.py index ac4e3be..7a2a0c7 100644 --- a/tests/unittests/objects/job/test_firewatch_objects_job_get_timestamp.py +++ b/tests/unittests/objects/job/test_firewatch_objects_job_get_timestamp.py @@ -6,8 +6,11 @@ class TestGetTimestamp(JobBaseTest): @patch("src.objects.job.storage.Client") - def test_get_timestamp(self, mock_storage_client): - # Mock the storage client and bucket + def test_get_timestamp_rehearsal_job(self, mock_storage_client): + """ + Tests that _get_timestamp uses the correct 'pr-logs' path for rehearsal jobs. + """ + # --- Setup Mocks --- mock_client = MagicMock() mock_bucket = MagicMock() mock_blob = MagicMock() @@ -15,10 +18,10 @@ def test_get_timestamp(self, mock_storage_client): mock_storage_client.return_value = mock_client mock_client.bucket.return_value = mock_bucket mock_bucket.blob.return_value = mock_blob - - # Mock the content of the started.json file mock_blob.download_as_text.return_value = '{"timestamp": 1617184800}' + # --- Create Rehearsal Job --- + # __init__ will set self.is_rehearsal = True and self.pr_id = "1234" job = Job( name="rehearse-1234-job1", name_safe="job1_safe", @@ -28,11 +31,61 @@ def test_get_timestamp(self, mock_storage_client): firewatch_config=self.config, ) + # --- Run Method --- timestamp = job._get_timestamp( - job_name="rehearse-1234-job1", build_id="123", storage_client=mock_client, gcs_bucket=mock_bucket + job_name="rehearse-1234-job1", + build_id="123", + storage_client=mock_client, + gcs_bucket="bucket1", # Pass the bucket name as a string ) + + # --- Assertions --- + # 1. Assert the return value is correct assert timestamp == 1617184800 + # 2. (CRITICAL) Assert the correct path was used + expected_path = "pr-logs/pull/openshift_release/1234/rehearse-1234-job1/123/started.json" + mock_bucket.blob.assert_called_with(expected_path) + + @patch("src.objects.job.storage.Client") + def test_get_timestamp_regular_job(self, mock_storage_client): + """ + Tests that _get_timestamp uses the original 'logs' path for regular jobs. + """ + # --- Setup Mocks --- + mock_client = MagicMock() + mock_bucket = MagicMock() + mock_blob = MagicMock() + + mock_storage_client.return_value = mock_client + mock_client.bucket.return_value = mock_bucket + mock_bucket.blob.return_value = mock_blob + mock_blob.download_as_text.return_value = '{"timestamp": 1617184800}' + + # --- Create Regular Job --- + # __init__ will set self.is_rehearsal = False + job = Job( + name="periodic-ci-job-1", + name_safe="job1_safe", + build_id="456", + gcs_bucket="bucket1", + gcs_creds_file=None, + firewatch_config=self.config, + ) + + # --- Run Method --- + timestamp = job._get_timestamp( + job_name="periodic-ci-job-1", build_id="456", storage_client=mock_client, gcs_bucket="bucket1" + ) + + # --- Assertions --- + # 1. Assert the return value is correct + assert timestamp == 1617184800 + + # 2. (CRITICAL) Assert the correct (original) path was used + expected_path = "logs/periodic-ci-job-1/456/started.json" + mock_bucket.blob.assert_called_with(expected_path) + if __name__ == "__main__": unittest.main()