Skip to content
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.

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

Add testcases to ensure that 'soft_fail' argument is respected when running ExternalTaskSensor #34652

Merged

Conversation

utkarsharma2
Copy link
Contributor

@utkarsharma2 utkarsharma2 commented Sep 27, 2023

This PR intends to add test cases to ensure that the soft_fail argument in ExternalTaskSensor is respected.

related: #34497, #33401

@boring-cyborg boring-cyborg bot added the area:core-operators Operators, Sensors and hooks within Core Airflow label Sep 27, 2023
@utkarsharma2 utkarsharma2 changed the title Respect 'soft_fail' argument when running ExternalTaskSensor Fix dag.test() and Respect 'soft_fail' argument when running ExternalTaskSensor Sep 27, 2023
Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

No need to add this patch in the core operators, we already fixed it, and for that we add the comment (TODO: remove this if block when min_airflow_version is set to higher than 2.7.1)

However, you can keep the tests (only the tests for the execute/patch method).

airflow/sensors/external_task.py Outdated Show resolved Hide resolved
@utkarsharma2
Copy link
Contributor Author

utkarsharma2 commented Sep 28, 2023

No need to add this patch in the core operators, we already fixed it, and for that we add the comment (TODO: remove this if block when min_airflow_version is set to higher than 2.7.1)

However, you can keep the tests (only the tests for the execute/patch method).

@hussein-awala I'm assuming you mean the entire code snippet when you said this patch

# TODO: remove this if block when min_airflow_version is set to higher than 2.7.1
message = ""
if self.soft_fail:
    raise AirflowSkipException(message)
raise AirflowException(message)

if so, _check_for_existence(self, session) was not respecting the soft_fail param and causing the dag.test() to halt the flow because AirflowException was raised instead of AirflowSkipException. So I'm not sure if it's handled.

or do you just mean the # TODO: remove this if block when min_airflow_version is set to higher than 2.7.1 by this patch?

@utkarsharma2 utkarsharma2 force-pushed the Soft_fail_ExternalTaskSensor branch from e3017dc to 8f306d1 Compare September 28, 2023 11:43
@utkarsharma2 utkarsharma2 changed the title Fix dag.test() and Respect 'soft_fail' argument when running ExternalTaskSensor Add testcases to ensure that 'soft_fail' argument is respected when running ExternalTaskSensor Sep 28, 2023
@utkarsharma2 utkarsharma2 requested a review from Lee-W September 28, 2023 12:05
@utkarsharma2 utkarsharma2 requested a review from Lee-W September 28, 2023 17:21
@hussein-awala hussein-awala added the type:misc/internal Changelog: Misc changes that should appear in change log label Oct 1, 2023
@hussein-awala hussein-awala merged commit 0fee730 into apache:main Oct 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core-operators Operators, Sensors and hooks within Core Airflow type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants