Skip to content

Conversation

@uranusjr
Copy link
Member

@uranusjr uranusjr commented Dec 8, 2025

This handles the DAGNode part for #52141.

Logic shared by both SDK and Core is implemented in a shared library. The shared class is implemented as a Generic, with concrete types (DAG, TaskGroup, etc.) supplied in Core and Task SDK.

I’m not sure what to do with tests… The functionalities are currently covered in SDK or Core tests. Do we pull them to the shared library? That would require us to add quite some code to stub out the concrete types that are unavailable in _shared.

@uranusjr uranusjr force-pushed the dagnode-shared branch 6 times, most recently from c54d91a to 84c6caa Compare December 8, 2025 23:05
@uranusjr uranusjr marked this pull request as ready for review December 8, 2025 23:05
@potiuk
Copy link
Member

potiuk commented Dec 9, 2025

I’m not sure what to do with tests… The functionalities are currently covered in SDK or Core tests. Do we pull them to the shared library? That would require us to add quite some code to stub out the concrete types that are unavailable in _shared.

Which tests for example ?

@uranusjr
Copy link
Member Author

uranusjr commented Dec 9, 2025

For example airflow-core/tests/unit/models/test_dag.py::TestTaskClearingSetupTeardownBehavior::test_get_flat_relative_ids_with_setup

This test really just tests get_flat_relative_ids_with_setup works as expected. This functon is implemented in the shared lib in this PR, but you can’t test the function on its own without an operator class. The same goes for everything in the class (aside from very trivial ones such as get_dag() that don’t really need tests anyway), and the package ends up having no tests at all (and it currently fails CI).

@uranusjr uranusjr force-pushed the dagnode-shared branch 2 times, most recently from 2f7e6e3 to 2164a6a Compare December 9, 2025 05:00
@uranusjr

This comment was marked as outdated.

@potiuk
Copy link
Member

potiuk commented Dec 16, 2025

For example airflow-core/tests/unit/models/test_dag.py::TestTaskClearingSetupTeardownBehavior::test_get_flat_relative_ids_with_setup

I think those tests should remain as they are - they are testing effectively integration of the methods with actual instances of Dag/Task for example.

I think however we could have mocked versions of the classes that would not be created by Dag/ / make_tests instantitaiton but simply using MagicMock objects with spec ?

@uranusjr uranusjr force-pushed the dagnode-shared branch 9 times, most recently from 9259187 to 12d9687 Compare December 22, 2025 05:40
@uranusjr
Copy link
Member Author

Redoing this in #59708.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants