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/02/27 08:43:00 UTC
[GitHub] [airflow] vemikhaylov opened a new pull request #14501: Fix set task instance form test
vemikhaylov opened a new pull request #14501:
URL: https://github.com/apache/airflow/pull/14501
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vemikhaylov commented on pull request #14501: Fix set task instance form test
Posted by GitBox <gi...@apache.org>.
vemikhaylov commented on pull request #14501:
URL: https://github.com/apache/airflow/pull/14501#issuecomment-787038751
Besides using the correct schema in the test, I removed the test for a bool value provided as a string. It had been failing as by default `marshmallow` successfully deserializes bool strings and numbers as well:
https://github.com/marshmallow-code/marshmallow/blob/7b5a78d77bc23ace331482817d3177147f4b0521/src/marshmallow/fields.py#L1080
If we like to be more rigid about that, we should change this behaviour for all the boolean fields in the API to be more consistent, I guess.
@mik-laj What do you think?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy merged pull request #14501: Fix set task instance form test
Posted by GitBox <gi...@apache.org>.
ephraimbuddy merged pull request #14501:
URL: https://github.com/apache/airflow/pull/14501
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #14501: Fix set task instance form test
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #14501:
URL: https://github.com/apache/airflow/pull/14501#discussion_r587283929
##########
File path: tests/api_connexion/schemas/test_task_instance_schema.py
##########
@@ -199,7 +199,6 @@ def test_success(self):
@parameterized.expand(
[
({"task_id": None},),
- ({"include_future": "True"},),
Review comment:
The OpenAPI spec validates these things for us. We shouldn't even be writing many of these Marshmallow schemas tests but since it's part of our code now and we are writing it, we should write it correctly.
And there's nothing specific about the `include_future`, it's just like other fields. Since we have tests for the other fields, let's also have a test for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vemikhaylov commented on a change in pull request #14501: Fix set task instance form test
Posted by GitBox <gi...@apache.org>.
vemikhaylov commented on a change in pull request #14501:
URL: https://github.com/apache/airflow/pull/14501#discussion_r587259860
##########
File path: tests/api_connexion/schemas/test_task_instance_schema.py
##########
@@ -199,7 +199,6 @@ def test_success(self):
@parameterized.expand(
[
({"task_id": None},),
- ({"include_future": "True"},),
Review comment:
To be quite honest, I'm not sure what exactly are we going to test by this thing? I suppose that we shouldn't test the Marshmallow functionality (i.e. correctness of `fields.Boolean` work) as it's not our area of responsibility. However, if we would like to do so, why not all the fields are tested on loading any wrong type with failure? What's specific about this `include_future` and `str` 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vemikhaylov commented on a change in pull request #14501: Fix set task instance form test
Posted by GitBox <gi...@apache.org>.
vemikhaylov commented on a change in pull request #14501:
URL: https://github.com/apache/airflow/pull/14501#discussion_r587259860
##########
File path: tests/api_connexion/schemas/test_task_instance_schema.py
##########
@@ -199,7 +199,6 @@ def test_success(self):
@parameterized.expand(
[
({"task_id": None},),
- ({"include_future": "True"},),
Review comment:
To be quite honest, I'm not sure what exactly are we going to test by this thing? I suppose that we shouldn't test the Marshmallow functionality (i.e. correctness of `fields.Boolean` work) as it's not our area of responsibility. However, if we would like to do so, why not all the fields are tested on loading any wrong type with failure? What's specific about these `include_future` and `str` 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vemikhaylov commented on a change in pull request #14501: Fix set task instance form test
Posted by GitBox <gi...@apache.org>.
vemikhaylov commented on a change in pull request #14501:
URL: https://github.com/apache/airflow/pull/14501#discussion_r587259860
##########
File path: tests/api_connexion/schemas/test_task_instance_schema.py
##########
@@ -199,7 +199,6 @@ def test_success(self):
@parameterized.expand(
[
({"task_id": None},),
- ({"include_future": "True"},),
Review comment:
To be quite honest, I'm not sure what exactly are we going to test by this thing? I suppose that we shouldn't test the Marshmallow functionality (i.e. correctness of `fields.Boolean` work) as it's not our area of responsibility. However, if we would like to do so, why not all the fields are tested on loading any wrong type with failure? What's specific about this `include_future` 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #14501: Fix set task instance form test
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #14501:
URL: https://github.com/apache/airflow/pull/14501#discussion_r586684031
##########
File path: tests/api_connexion/schemas/test_task_instance_schema.py
##########
@@ -199,7 +199,6 @@ def test_success(self):
@parameterized.expand(
[
({"task_id": None},),
- ({"include_future": "True"},),
Review comment:
We shouldn't delete this, rather we should use marshmallow none truthy string. Please add it back and replace "True" with "mystring"
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] vemikhaylov commented on a change in pull request #14501: Fix set task instance form test
Posted by GitBox <gi...@apache.org>.
vemikhaylov commented on a change in pull request #14501:
URL: https://github.com/apache/airflow/pull/14501#discussion_r587331823
##########
File path: tests/api_connexion/schemas/test_task_instance_schema.py
##########
@@ -199,7 +199,6 @@ def test_success(self):
@parameterized.expand(
[
({"task_id": None},),
- ({"include_future": "True"},),
Review comment:
Well, no problem, I returned it back.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #14501: Fix set task instance form test
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #14501:
URL: https://github.com/apache/airflow/pull/14501#discussion_r587283929
##########
File path: tests/api_connexion/schemas/test_task_instance_schema.py
##########
@@ -199,7 +199,6 @@ def test_success(self):
@parameterized.expand(
[
({"task_id": None},),
- ({"include_future": "True"},),
Review comment:
The OpenAPI spec validates these things for us. We shouldn't even be writing many of these Marshmallow schemas but since it's part of our code now and we are writing it, we should write it correctly.
And there's nothing specific about the `include_future`, it's just like other fields. Since we have tests for the other fields, let's also have a test for 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org