Skip to content

Fix path & rehersal job type check in some methods & improve related unit tests #264

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dfrazzette merged 1 commit into from
Nov 12, 2025
Merged

Fix path & rehersal job type check in some methods & improve related unit tests #264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions src/objects/job.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,11 +312,14 @@ def _get_steps(

steps = []

if self.is_rehearsal:
Copy link

@etirta etirta Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen several pattern are repeated, branching and settings. This is maintenance burden.

A much better approach is to branch 1x and store the deviating data once.

Say at line 49:

        if self.is_rehearsal:
            try:
                self.pr_id = pr_id or os.getenv("PULL_NUMBER") or self.name.split("-")[1]  # type: ignore
            except IndexError:
                self.logger.warning(
                    f"Pull number for job {self.name} not obtained, reporting may not be complete.",
                )
                self.pr_id = "1"
            self.logger.info(f"PR ID: {self.pr_id}")
            self.blobPfx = "pr-logs/pull/openshift_release/{self.pr_id}"
        else:
            self.pr_id = ""
            self.blobPfx = "logs"

Then we can safe us a lot of branches throughout the code and if any change on the path is only 1 place modification.

Another observation, not really related to this PR, why a lot of function parameters, such as job_name is marked with Optional[...] hint, while it's clearly inside the function code it can' handle None type? This is not good practice.

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]
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 = []
Expand Down
74 changes: 65 additions & 9 deletions tests/unittests/objects/job/test_firewatch_objects_job_get_all_build_ids.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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__":
Expand Down
63 changes: 58 additions & 5 deletions tests/unittests/objects/job/test_firewatch_objects_job_get_timestamp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,22 @@

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()

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",
Expand All @@ -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()
Loading