Skip to content

Conversation

@aviatco
Copy link
Collaborator

@aviatco aviatco commented Jan 12, 2026

✨ Description of new changes

Fix #128
This pull request fixes the behavior of the job run command so that it properly exits with status code 1 when a job fails, and adds comprehensive unit tests to verify error handling. The main changes focus on error reporting and test coverage for job failures.

Error handling improvements:

  • Modified wait_for_job_completion in fab_cmd_job_utils.py to raise a FabricCLIError with ERROR_JOB_FAILED when a job status is 'Failed', ensuring the CLI exits with code 1 on failure.
  • Updated the unreleased changes log to document the fix for proper exit status on job run failure. (.changes/unreleased/fixed-20260112-143553.yaml)

Testing enhancements:

  • Added three new unit tests to test_jobs.py to verify that job failures (including with a timeout) correctly raise FabricCLIError and result in exit code 1, improving confidence in error handling for failed jobs.
  • Imported FabricCLIError in test_jobs.py to support new failure-handling tests.

@aviatco aviatco requested a review from a team as a code owner January 12, 2026 14:38
assert exc_info.value.status_code == constant.ERROR_JOB_FAILED
assert "Job instance 'test-job-instance-id-timeout' Failed" in str(exc_info.value.message)

def test_job_failure_error_handling_unit_test(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we record the tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is nothing to record here since this is unit test... we mock everything and tests the function wait_for_job_completion - maybe this is not the right place for those tests?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, if we have mocks and only tests a component within the cli and not a command, it should be placed under test_core or test_utils

Copy link
Collaborator

Choose a reason for hiding this comment

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

@aviatco we should move this to unit tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] fab run doesn't return the correct exit code on error

2 participants