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 2021/08/04 22:58:06 UTC
[GitHub] [airflow] ephraimbuddy opened a new pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
ephraimbuddy opened a new pull request #17265:
URL: https://github.com/apache/airflow/pull/17265
This change adds `dag_maker` fixture to test_scheduler_job.py. This
will help us create `dagruns` each time we create a dag
The dag_maker fixture was also updated
---
**^ 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 change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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] ashb commented on a change in pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #17265:
URL: https://github.com/apache/airflow/pull/17265#discussion_r678972149
##########
File path: tests/jobs/test_scheduler_job.py
##########
@@ -293,11 +295,7 @@ def test_execute_task_instances_is_paused_wont_execute(self):
max_active_tasks=dag.max_active_tasks,
has_task_concurrency_limits=False,
)
- dr1 = dag.create_dagrun(
- run_type=DagRunType.BACKFILL_JOB,
- execution_date=DEFAULT_DATE,
- state=State.RUNNING,
- )
+ dr1 = dag_maker.dag_run
Review comment:
Thinking about my comment about "two" factories, maybe _this_ is how we could have it in one.
```
dag_maker.make_dag_run(dag_run_args_go="here")
```
That way args to `dag_maker()` are _only_ for the dag, and the DagRun args are separate here.
And for simplicity/common case, we can have
```
@property
def dag_run(self):
return self.make_dag_run()
```
--
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 change in pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17265:
URL: https://github.com/apache/airflow/pull/17265#discussion_r682623150
##########
File path: tests/jobs/test_scheduler_job.py
##########
@@ -119,10 +119,18 @@ def clean_db():
# DO NOT try to run clear_db_serialized_dags() here - this will break the tests
# The tests expect DAGs to be fully loaded here via setUpClass method below
+ def setup_method(self):
+ self.clean_db()
+
+ def teardown_method(self):
+ if self.scheduler_job and self.scheduler_job.processor_agent:
+ self.scheduler_job.processor_agent.end()
+ self.scheduler_job = None
+ self.clean_db()
Review comment:
Any particualr reason for this change? This does not seem to change things functionality-wise afaict
--
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] ephraimbuddy commented on a change in pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #17265:
URL: https://github.com/apache/airflow/pull/17265#discussion_r678010789
##########
File path: tests/jobs/test_scheduler_job.py
##########
@@ -1242,12 +1165,12 @@ def test_execute_task_instances_unlimited(self):
date = dag.start_date
tis = []
for _ in range(0, 20):
+ date = dag.following_schedule(date)
dr = dag.create_dagrun(
run_type=DagRunType.SCHEDULED,
execution_date=date,
state=State.RUNNING,
)
- date = dag.following_schedule(date)
Review comment:
Not really. What’s happening is that it’s now skipping creation of the first dagrun because the dag_maker fixture has created it. Without this, it’ll fail on integrity error while trying to create the same dagrun already created by fixture
--
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 change in pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17265:
URL: https://github.com/apache/airflow/pull/17265#discussion_r678028882
##########
File path: tests/jobs/test_scheduler_job.py
##########
@@ -1242,12 +1165,12 @@ def test_execute_task_instances_unlimited(self):
date = dag.start_date
tis = []
for _ in range(0, 20):
+ date = dag.following_schedule(date)
dr = dag.create_dagrun(
run_type=DagRunType.SCHEDULED,
execution_date=date,
state=State.RUNNING,
)
- date = dag.following_schedule(date)
Review comment:
I see. Thanks.
--
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] ephraimbuddy closed pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
ephraimbuddy closed pull request #17265:
URL: https://github.com/apache/airflow/pull/17265
--
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] ashb commented on pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #17265:
URL: https://github.com/apache/airflow/pull/17265#issuecomment-895446550
🎉
--
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] ashb commented on a change in pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #17265:
URL: https://github.com/apache/airflow/pull/17265#discussion_r678970064
##########
File path: tests/conftest.py
##########
@@ -444,11 +445,17 @@ def __exit__(self, type, value, traceback):
dag.__exit__(type, value, traceback)
if type is None:
dag.clear()
+
self.dag_run = dag.create_dagrun(
run_id=self.kwargs.get("run_id", "test"),
state=self.kwargs.get('state', State.RUNNING),
execution_date=self.kwargs.get('execution_date', self.kwargs['start_date']),
start_date=self.kwargs['start_date'],
+ run_type=self.kwargs.get('run_type', DagRunType.SCHEDULED),
+ conf=self.kwargs.get('conf', {}),
+ dag_hash=self.kwargs.get('dag_hash', None),
+ external_trigger=self.kwargs.get('external_trigger', False),
+ creating_job_id=self.kwargs.get('creating_job_id', None),
Review comment:
I think we should have two factories: `dag_and_dagrun_creator` (though I don't love the name) and a `dag_maker` -- `dag_maker` would be responsible for makeing the DAG, and ensuring that the SerliazedDag and DagModel tables are up-to-date, and then the `dag_and_dagrun_creator` can use that factory somehow, but also create a DagRun.
--
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] ephraimbuddy closed pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
ephraimbuddy closed pull request #17265:
URL: https://github.com/apache/airflow/pull/17265
--
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 change in pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17265:
URL: https://github.com/apache/airflow/pull/17265#discussion_r677981562
##########
File path: tests/conftest.py
##########
@@ -444,11 +445,17 @@ def __exit__(self, type, value, traceback):
dag.__exit__(type, value, traceback)
if type is None:
dag.clear()
+
self.dag_run = dag.create_dagrun(
run_id=self.kwargs.get("run_id", "test"),
state=self.kwargs.get('state', State.RUNNING),
execution_date=self.kwargs.get('execution_date', self.kwargs['start_date']),
start_date=self.kwargs['start_date'],
+ run_type=self.kwargs.get('run_type', DagRunType.SCHEDULED),
+ conf=self.kwargs.get('conf', {}),
+ dag_hash=self.kwargs.get('dag_hash', None),
+ external_trigger=self.kwargs.get('external_trigger', False),
+ creating_job_id=self.kwargs.get('creating_job_id', None),
Review comment:
I wonder if it’s better to store the kwargs explicitly instead. Either directly on `DagFactory`, or with a `NamedTuple` or `TypedDict`. This is getting unwieldy.
--
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 change in pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17265:
URL: https://github.com/apache/airflow/pull/17265#discussion_r684942112
##########
File path: tests/jobs/test_scheduler_job.py
##########
@@ -495,45 +439,24 @@ def test_find_executable_task_instances_pool(self):
assert tis[3].key in res_keys
session.rollback()
- def test_find_executable_task_instances_order_execution_date(self):
+ def test_find_executable_task_instances_order_execution_date(self, dag_maker):
dag_id_1 = 'SchedulerJobTest.test_find_executable_task_instances_order_execution_date-a'
dag_id_2 = 'SchedulerJobTest.test_find_executable_task_instances_order_execution_date-b'
task_id = 'task-a'
- dag_1 = DAG(dag_id=dag_id_1, start_date=DEFAULT_DATE, max_active_tasks=16)
- dag_2 = DAG(dag_id=dag_id_2, start_date=DEFAULT_DATE, max_active_tasks=16)
- dag1_task = DummyOperator(dag=dag_1, task_id=task_id)
- dag2_task = DummyOperator(dag=dag_2, task_id=task_id)
+ with dag_maker(dag_id=dag_id_1, max_active_tasks=16) as dag_1:
+ dag1_task = DummyOperator(task_id=task_id)
+ dr1 = dag_maker.create_dagrun(execution_date=DEFAULT_DATE + timedelta(hours=1))
Review comment:
I think the original setup was trying to test that if the `DEFAULT_DATE + 1h` run is (somehow) scheduled before the `DEFAULT_DATE` one, tasks in the earlier run will still be run first. It’s probably worthwhile to add a docstring or comment describing the test’s setup.
--
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] ephraimbuddy commented on a change in pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #17265:
URL: https://github.com/apache/airflow/pull/17265#discussion_r678011711
##########
File path: tests/conftest.py
##########
@@ -444,11 +445,17 @@ def __exit__(self, type, value, traceback):
dag.__exit__(type, value, traceback)
if type is None:
dag.clear()
+
self.dag_run = dag.create_dagrun(
run_id=self.kwargs.get("run_id", "test"),
state=self.kwargs.get('state', State.RUNNING),
execution_date=self.kwargs.get('execution_date', self.kwargs['start_date']),
start_date=self.kwargs['start_date'],
+ run_type=self.kwargs.get('run_type', DagRunType.SCHEDULED),
+ conf=self.kwargs.get('conf', {}),
+ dag_hash=self.kwargs.get('dag_hash', None),
+ external_trigger=self.kwargs.get('external_trigger', False),
+ creating_job_id=self.kwargs.get('creating_job_id', None),
Review comment:
It’s just all the arguments that dag.create_dagrun takes, I’m thinking since it’s just test we should leave it like 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] ephraimbuddy commented on pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #17265:
URL: https://github.com/apache/airflow/pull/17265#issuecomment-895455484
> 🎉
Thanks for your helps!
--
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] ashb commented on a change in pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #17265:
URL: https://github.com/apache/airflow/pull/17265#discussion_r683294385
##########
File path: tests/jobs/test_scheduler_job.py
##########
@@ -495,45 +439,24 @@ def test_find_executable_task_instances_pool(self):
assert tis[3].key in res_keys
session.rollback()
- def test_find_executable_task_instances_order_execution_date(self):
+ def test_find_executable_task_instances_order_execution_date(self, dag_maker):
dag_id_1 = 'SchedulerJobTest.test_find_executable_task_instances_order_execution_date-a'
dag_id_2 = 'SchedulerJobTest.test_find_executable_task_instances_order_execution_date-b'
task_id = 'task-a'
- dag_1 = DAG(dag_id=dag_id_1, start_date=DEFAULT_DATE, max_active_tasks=16)
- dag_2 = DAG(dag_id=dag_id_2, start_date=DEFAULT_DATE, max_active_tasks=16)
- dag1_task = DummyOperator(dag=dag_1, task_id=task_id)
- dag2_task = DummyOperator(dag=dag_2, task_id=task_id)
+ with dag_maker(dag_id=dag_id_1, max_active_tasks=16) as dag_1:
+ dag1_task = DummyOperator(task_id=task_id)
+ dr1 = dag_maker.create_dagrun(execution_date=DEFAULT_DATE + timedelta(hours=1))
Review comment:
It may not be relevant, but why does the execution_date change here?
--
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 change in pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17265:
URL: https://github.com/apache/airflow/pull/17265#discussion_r682623150
##########
File path: tests/jobs/test_scheduler_job.py
##########
@@ -119,10 +119,18 @@ def clean_db():
# DO NOT try to run clear_db_serialized_dags() here - this will break the tests
# The tests expect DAGs to be fully loaded here via setUpClass method below
+ def setup_method(self):
+ self.clean_db()
+
+ def teardown_method(self):
+ if self.scheduler_job and self.scheduler_job.processor_agent:
+ self.scheduler_job.processor_agent.end()
+ self.scheduler_job = None
+ self.clean_db()
Review comment:
Any particualr reason for this change? This does not seem to change things functionality-wise afaict
--
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] ephraimbuddy commented on a change in pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #17265:
URL: https://github.com/apache/airflow/pull/17265#discussion_r682638806
##########
File path: tests/jobs/test_scheduler_job.py
##########
@@ -119,10 +119,18 @@ def clean_db():
# DO NOT try to run clear_db_serialized_dags() here - this will break the tests
# The tests expect DAGs to be fully loaded here via setUpClass method below
+ def setup_method(self):
+ self.clean_db()
+
+ def teardown_method(self):
+ if self.scheduler_job and self.scheduler_job.processor_agent:
+ self.scheduler_job.processor_agent.end()
+ self.scheduler_job = None
+ self.clean_db()
Review comment:
I had error while trying to delete DagModel in a previous push but I can see now that here is not the problem so, I will revert it
--
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 change in pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17265:
URL: https://github.com/apache/airflow/pull/17265#discussion_r677991151
##########
File path: tests/jobs/test_scheduler_job.py
##########
@@ -1242,12 +1165,12 @@ def test_execute_task_instances_unlimited(self):
date = dag.start_date
tis = []
for _ in range(0, 20):
+ date = dag.following_schedule(date)
dr = dag.create_dagrun(
run_type=DagRunType.SCHEDULED,
execution_date=date,
state=State.RUNNING,
)
- date = dag.following_schedule(date)
Review comment:
Hmm, I think this changes the behaviour…?
--
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] ashb commented on a change in pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #17265:
URL: https://github.com/apache/airflow/pull/17265#discussion_r683294385
##########
File path: tests/jobs/test_scheduler_job.py
##########
@@ -495,45 +439,24 @@ def test_find_executable_task_instances_pool(self):
assert tis[3].key in res_keys
session.rollback()
- def test_find_executable_task_instances_order_execution_date(self):
+ def test_find_executable_task_instances_order_execution_date(self, dag_maker):
dag_id_1 = 'SchedulerJobTest.test_find_executable_task_instances_order_execution_date-a'
dag_id_2 = 'SchedulerJobTest.test_find_executable_task_instances_order_execution_date-b'
task_id = 'task-a'
- dag_1 = DAG(dag_id=dag_id_1, start_date=DEFAULT_DATE, max_active_tasks=16)
- dag_2 = DAG(dag_id=dag_id_2, start_date=DEFAULT_DATE, max_active_tasks=16)
- dag1_task = DummyOperator(dag=dag_1, task_id=task_id)
- dag2_task = DummyOperator(dag=dag_2, task_id=task_id)
+ with dag_maker(dag_id=dag_id_1, max_active_tasks=16) as dag_1:
+ dag1_task = DummyOperator(task_id=task_id)
+ dr1 = dag_maker.create_dagrun(execution_date=DEFAULT_DATE + timedelta(hours=1))
Review comment:
It may not be relevant, but why does the execution_date change here?
--
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] ashb commented on pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #17265:
URL: https://github.com/apache/airflow/pull/17265#issuecomment-895142979
I can't really work out what is going on with the test failure, it's consistent but doesn't make sense.
But anyway, those tests don't belong in test_scheduler_job anyway, so in https://github.com/apache/airflow/pull/17504 I moved it.
--
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] ephraimbuddy commented on a change in pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #17265:
URL: https://github.com/apache/airflow/pull/17265#discussion_r683299361
##########
File path: tests/jobs/test_scheduler_job.py
##########
@@ -495,45 +439,24 @@ def test_find_executable_task_instances_pool(self):
assert tis[3].key in res_keys
session.rollback()
- def test_find_executable_task_instances_order_execution_date(self):
+ def test_find_executable_task_instances_order_execution_date(self, dag_maker):
dag_id_1 = 'SchedulerJobTest.test_find_executable_task_instances_order_execution_date-a'
dag_id_2 = 'SchedulerJobTest.test_find_executable_task_instances_order_execution_date-b'
task_id = 'task-a'
- dag_1 = DAG(dag_id=dag_id_1, start_date=DEFAULT_DATE, max_active_tasks=16)
- dag_2 = DAG(dag_id=dag_id_2, start_date=DEFAULT_DATE, max_active_tasks=16)
- dag1_task = DummyOperator(dag=dag_1, task_id=task_id)
- dag2_task = DummyOperator(dag=dag_2, task_id=task_id)
+ with dag_maker(dag_id=dag_id_1, max_active_tasks=16) as dag_1:
+ dag1_task = DummyOperator(task_id=task_id)
+ dr1 = dag_maker.create_dagrun(execution_date=DEFAULT_DATE + timedelta(hours=1))
Review comment:
It's how the dagrun was created before this change,. I decided not to change it. Look at the deleted line 528
--
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 change in pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17265:
URL: https://github.com/apache/airflow/pull/17265#discussion_r678029416
##########
File path: tests/conftest.py
##########
@@ -444,11 +445,17 @@ def __exit__(self, type, value, traceback):
dag.__exit__(type, value, traceback)
if type is None:
dag.clear()
+
self.dag_run = dag.create_dagrun(
run_id=self.kwargs.get("run_id", "test"),
state=self.kwargs.get('state', State.RUNNING),
execution_date=self.kwargs.get('execution_date', self.kwargs['start_date']),
start_date=self.kwargs['start_date'],
+ run_type=self.kwargs.get('run_type', DagRunType.SCHEDULED),
+ conf=self.kwargs.get('conf', {}),
+ dag_hash=self.kwargs.get('dag_hash', None),
+ external_trigger=self.kwargs.get('external_trigger', False),
+ creating_job_id=self.kwargs.get('creating_job_id', None),
Review comment:
Test code is still code 🙂 But I’m not too strong on this; we can always fix things later when they break.
--
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] ephraimbuddy commented on a change in pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #17265:
URL: https://github.com/apache/airflow/pull/17265#discussion_r678037509
##########
File path: tests/conftest.py
##########
@@ -444,11 +445,17 @@ def __exit__(self, type, value, traceback):
dag.__exit__(type, value, traceback)
if type is None:
dag.clear()
+
self.dag_run = dag.create_dagrun(
run_id=self.kwargs.get("run_id", "test"),
state=self.kwargs.get('state', State.RUNNING),
execution_date=self.kwargs.get('execution_date', self.kwargs['start_date']),
start_date=self.kwargs['start_date'],
+ run_type=self.kwargs.get('run_type', DagRunType.SCHEDULED),
+ conf=self.kwargs.get('conf', {}),
+ dag_hash=self.kwargs.get('dag_hash', None),
+ external_trigger=self.kwargs.get('external_trigger', False),
+ creating_job_id=self.kwargs.get('creating_job_id', None),
Review comment:
Cool. I'll try the NamedTuple when I rename the `dag_maker` to something like `dag_and_dagrun_creator` to be more reflective
--
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] ephraimbuddy commented on a change in pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #17265:
URL: https://github.com/apache/airflow/pull/17265#discussion_r682638806
##########
File path: tests/jobs/test_scheduler_job.py
##########
@@ -119,10 +119,18 @@ def clean_db():
# DO NOT try to run clear_db_serialized_dags() here - this will break the tests
# The tests expect DAGs to be fully loaded here via setUpClass method below
+ def setup_method(self):
+ self.clean_db()
+
+ def teardown_method(self):
+ if self.scheduler_job and self.scheduler_job.processor_agent:
+ self.scheduler_job.processor_agent.end()
+ self.scheduler_job = None
+ self.clean_db()
Review comment:
I had error while trying to delete DagModel in a previous push but I can see now that here is not the problem so, I will revert it
##########
File path: tests/jobs/test_scheduler_job.py
##########
@@ -495,45 +439,24 @@ def test_find_executable_task_instances_pool(self):
assert tis[3].key in res_keys
session.rollback()
- def test_find_executable_task_instances_order_execution_date(self):
+ def test_find_executable_task_instances_order_execution_date(self, dag_maker):
dag_id_1 = 'SchedulerJobTest.test_find_executable_task_instances_order_execution_date-a'
dag_id_2 = 'SchedulerJobTest.test_find_executable_task_instances_order_execution_date-b'
task_id = 'task-a'
- dag_1 = DAG(dag_id=dag_id_1, start_date=DEFAULT_DATE, max_active_tasks=16)
- dag_2 = DAG(dag_id=dag_id_2, start_date=DEFAULT_DATE, max_active_tasks=16)
- dag1_task = DummyOperator(dag=dag_1, task_id=task_id)
- dag2_task = DummyOperator(dag=dag_2, task_id=task_id)
+ with dag_maker(dag_id=dag_id_1, max_active_tasks=16) as dag_1:
+ dag1_task = DummyOperator(task_id=task_id)
+ dr1 = dag_maker.create_dagrun(execution_date=DEFAULT_DATE + timedelta(hours=1))
Review comment:
It's how the dagrun was created before this change,. I decided not to change it. Look at the deleted line 528
--
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] ashb commented on a change in pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #17265:
URL: https://github.com/apache/airflow/pull/17265#discussion_r678974000
##########
File path: tests/jobs/test_scheduler_job.py
##########
@@ -293,11 +295,7 @@ def test_execute_task_instances_is_paused_wont_execute(self):
max_active_tasks=dag.max_active_tasks,
has_task_concurrency_limits=False,
)
Review comment:
This block here for instance where we create a DagModel appears 23 times in this file, so this should be moved in side the DagMaker
--
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] ephraimbuddy closed pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
ephraimbuddy closed pull request #17265:
URL: https://github.com/apache/airflow/pull/17265
--
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] ephraimbuddy merged pull request #17265: Use `dag_maker` fixture in test_scheduler_job.py
Posted by GitBox <gi...@apache.org>.
ephraimbuddy merged pull request #17265:
URL: https://github.com/apache/airflow/pull/17265
--
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