You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2023/01/04 10:38:05 UTC
[GitHub] [airflow] Soonmok opened a new pull request, #28724: Use dagrun instead of DagFileProcessor in backfill_job test
Soonmok opened a new pull request, #28724:
URL: https://github.com/apache/airflow/pull/28724
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
related: #23539
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
Fix xfail test in `tests/jobs/test_backfill_job.py::test_trigger_controller_dag` test using `dagrun`, instead of `DagFileProcessor` which is deprecated
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis merged pull request #28724: Use dagrun instead of DagFileProcessor in backfill_job test
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis merged PR #28724:
URL: https://github.com/apache/airflow/pull/28724
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #28724: Use dagrun instead of DagFileProcessor in backfill_job test
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #28724:
URL: https://github.com/apache/airflow/pull/28724#discussion_r1063179968
##########
tests/jobs/test_backfill_job.py:
##########
@@ -147,31 +147,25 @@ def test_dag_run_with_finished_tasks_set_to_success(self, dag_maker):
assert State.SUCCESS == dag_run.state
- @pytest.mark.xfail(condition=True, reason="This test is flaky")
@pytest.mark.backend("postgres", "mysql")
def test_trigger_controller_dag(self):
Review Comment:
```suggestion
def test_trigger_controller_dag(self, session):
```
you can do this to get rid of the `session = settings.Session()` line.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #28724: Use dagrun instead of DagFileProcessor in backfill_job test
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #28724:
URL: https://github.com/apache/airflow/pull/28724#discussion_r1073117006
##########
tests/jobs/test_backfill_job.py:
##########
@@ -147,31 +147,24 @@ def test_dag_run_with_finished_tasks_set_to_success(self, dag_maker):
assert State.SUCCESS == dag_run.state
- @pytest.mark.xfail(condition=True, reason="This test is flaky")
@pytest.mark.backend("postgres", "mysql")
- def test_trigger_controller_dag(self):
+ def test_trigger_controller_dag(self, session):
dag = self.dagbag.get_dag("example_trigger_controller_dag")
target_dag = self.dagbag.get_dag("example_trigger_target_dag")
target_dag.sync_to_db()
- # dag_file_processor = DagFileProcessor(dag_ids=[], log=Mock())
- task_instances_list = []
- # task_instances_list = dag_file_processor._process_task_instances(
- # target_dag,
- # dag_runs=DagRun.find(dag_id='example_trigger_target_dag')
- # )
- assert not task_instances_list
+ target_dag_run = session.query(DagRun).filter(DagRun.dag_id == target_dag.dag_id).one_or_none()
+ assert not target_dag_run
job = BackfillJob(
dag=dag, start_date=DEFAULT_DATE, end_date=DEFAULT_DATE, ignore_first_depends_on_past=True
)
job.run()
- task_instances_list = []
- # task_instances_list = dag_file_processor._process_task_instances(
- # target_dag,
- # dag_runs=DagRun.find(dag_id='example_trigger_target_dag')
- # )
+ dag_run = session.query(DagRun).filter(DagRun.dag_id == dag.dag_id).one_or_none()
+ assert dag_run
Review Comment:
```suggestion
assert dag_run is not None
```
##########
tests/jobs/test_backfill_job.py:
##########
@@ -147,31 +147,24 @@ def test_dag_run_with_finished_tasks_set_to_success(self, dag_maker):
assert State.SUCCESS == dag_run.state
- @pytest.mark.xfail(condition=True, reason="This test is flaky")
@pytest.mark.backend("postgres", "mysql")
- def test_trigger_controller_dag(self):
+ def test_trigger_controller_dag(self, session):
dag = self.dagbag.get_dag("example_trigger_controller_dag")
target_dag = self.dagbag.get_dag("example_trigger_target_dag")
target_dag.sync_to_db()
- # dag_file_processor = DagFileProcessor(dag_ids=[], log=Mock())
- task_instances_list = []
- # task_instances_list = dag_file_processor._process_task_instances(
- # target_dag,
- # dag_runs=DagRun.find(dag_id='example_trigger_target_dag')
- # )
- assert not task_instances_list
+ target_dag_run = session.query(DagRun).filter(DagRun.dag_id == target_dag.dag_id).one_or_none()
+ assert not target_dag_run
Review Comment:
```suggestion
assert target_dag_run is None
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Soonmok commented on pull request #28724: Use dagrun instead of DagFileProcessor in backfill_job test
Posted by GitBox <gi...@apache.org>.
Soonmok commented on PR #28724:
URL: https://github.com/apache/airflow/pull/28724#issuecomment-1372031775
> Also, could you do some history digging (`git blame`) to find when the xfail was added, and if there’s any reasoning associated with the addition.
#8015 PR was related
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #28724: Use dagrun instead of DagFileProcessor in backfill_job test
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #28724:
URL: https://github.com/apache/airflow/pull/28724#issuecomment-1435765714
Awesome work, congrats on your first merged pull request!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28724: Use dagrun instead of DagFileProcessor in backfill_job test
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #28724:
URL: https://github.com/apache/airflow/pull/28724#issuecomment-1385326749
Static checks need fixing/rebase is needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28724: Use dagrun instead of DagFileProcessor in backfill_job test
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #28724:
URL: https://github.com/apache/airflow/pull/28724#issuecomment-1386756964
Needs rebase/fixing static checks,
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #28724: Use dagrun instead of DagFileProcessor in backfill_job test
Posted by GitBox <gi...@apache.org>.
uranusjr commented on code in PR #28724:
URL: https://github.com/apache/airflow/pull/28724#discussion_r1062153339
##########
tests/jobs/test_backfill_job.py:
##########
@@ -147,31 +147,22 @@ def test_dag_run_with_finished_tasks_set_to_success(self, dag_maker):
assert State.SUCCESS == dag_run.state
- @pytest.mark.xfail(condition=True, reason="This test is flaky")
@pytest.mark.backend("postgres", "mysql")
def test_trigger_controller_dag(self):
dag = self.dagbag.get_dag("example_trigger_controller_dag")
target_dag = self.dagbag.get_dag("example_trigger_target_dag")
target_dag.sync_to_db()
- # dag_file_processor = DagFileProcessor(dag_ids=[], log=Mock())
- task_instances_list = []
- # task_instances_list = dag_file_processor._process_task_instances(
- # target_dag,
- # dag_runs=DagRun.find(dag_id='example_trigger_target_dag')
- # )
- assert not task_instances_list
+ target_dag_run = target_dag.get_last_dagrun()
Review Comment:
Instead of relying on `get_last_dagrun()` (which can have bugs), it could be better to use SQLAlchemy directly here. There is a `session` pytest fixture you can use for this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #28724: Use dagrun instead of DagFileProcessor in backfill_job test
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #28724:
URL: https://github.com/apache/airflow/pull/28724#issuecomment-1371841783
Also, could you do some history digging (`git blame`) to find when the xfail was added, and if there’s any reasoning associated with the addition.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org