SkipMixin: Add missing session.commit() and test

[yuqian90]

I don't know the reason, but if we omit the session.commit() that I just added, test_skip_all_except fails because task3 state is None instead of SKIPPED. I think it's an issue with sqlalchemy or sqlite and not much to do with Airflow itself.

(NOTE: The impact of this bug being fixed here is pretty minimal and hard to catch because even though this line does not set the state to SKIPPED, the newly introduced NotPreviouslySkippedDep actually causes the scheduler to skip the task that are supposed to be skipped. So to the user, this bug being fixed here had almost no impact other than in some test cases. But this session.commit() is needed in case in the future someone calls skip_all_except() and expects the state to be set to SKIPPED for the skipped tests).

[yuqian90]

Hi @kaxil this bug affects 1.10.12rc3 (even though it's a pretty minor issue). I'm hoping this can be merged into both master and the upcoming 1.10.12 release.

[potiuk]

The reason is this: https://github.com/apache/airflow/blob/master/airflow/models/xcom.py#L101 . The xcom_push method performs "expunge_all()" which removes all modified object from the session. Not sure why though :)

@kaxill - I believe it''s not a case for 1.10.12rc4, but we should definitely merge it if there are other changes. :)

[yuqian90]

The reason is this: https://github.com/apache/airflow/blob/master/airflow/models/xcom.py#L101 . The xcom_push method performs "expunge_all()" which removes all modified object from the session. Not sure why though :)

@kaxill - I believe it''s not a case for 1.10.12rc4, but we should definitely merge it if there are other changes. :)

I see. Thanks @potiuk for pointing out!

[kaxil]

I would say this can wait for 1.10.13. If we have a major bug report (hopefully not 🤞 ) for 1.10.12rc3 -- then we can include this for rc4 -- but I seriously hope not :D

[potiuk]

Marked it as 1.10.13 then :)