Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "Check task attribute before use in sentry.add_tagging() (#37143)" #38519

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

Taragolis
Copy link
Contributor

This reverts commit 77d2fc7.

Reason for reverts:

  1. Test placed into the tests/test_sentry.py which do not pick up by selective checks, it should be placed into the tests/core/test_sentry.py
  2. Test do not pass, first with mocking context manager, even with fix this one by replace mock_configure_scope.return_value to mock_configure_scope.return_value.__enter__.return_value it starts fail with mock assertion errors
  3. Test use inclusive words
root@6eb4af1c407d:/opt/airflow# pytest tests/test_sentry.py
====================================================================== test session starts =======================================================================
platform linux -- Python 3.8.19, pytest-7.4.4, pluggy-1.4.0 -- /usr/local/bin/python
cachedir: .pytest_cache
rootdir: /opt/airflow
configfile: pyproject.toml
plugins: custom-exit-code-0.3.0, instafail-0.5.0, rerunfailures-14.0, asyncio-0.23.6, cov-5.0.0, xdist-3.5.0, timeouts-1.2.1, anyio-4.3.0, icdiff-0.9, mock-3.14.0, requests-mock-1.11.0, time-machine-2.14.1
asyncio: mode=strict
setup timeout: 0.0s, execution timeout: 0.0s, teardown timeout: 0.0s
collected 1 item                                                                                                                                                 

tests/test_sentry.py::test_configured_sentry_add_tagging FAILED                                                                                            [100%]

============================================================================ FAILURES ============================================================================
_______________________________________________________________ test_configured_sentry_add_tagging _______________________________________________________________

mock_configure_scope = <MagicMock name='configure_scope' id='281473541367312'>

    @patch("sentry_sdk.configure_scope")
    def test_configured_sentry_add_tagging(mock_configure_scope):
        mock_scope = create_autospec(Scope)
        mock_configure_scope.return_value = mock_scope
    
        from airflow.sentry.configured import ConfiguredSentry
    
        sentry = ConfiguredSentry()
    
        dummy_tags = ConfiguredSentry.SCOPE_DAG_RUN_TAGS | ConfiguredSentry.SCOPE_TASK_INSTANCE_TAGS
    
        # It should not raise error with both "dag_run" and "task" attributes, available.
        task_instance_1 = create_autospec(TaskInstance)
        task_instance_1.dag_run = MagicMock()
        task_instance_1.task = "task_1"
        for tag in dummy_tags:
            setattr(task_instance_1.dag_run, tag, "dummy")
>       sentry.add_tagging(task_instance_1)

tests/test_sentry.py:44: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <airflow.sentry.configured.ConfiguredSentry object at 0xffffaa74faf0>, task_instance = <MagicMock spec='TaskInstance' id='281473497709872'>

    def add_tagging(self, task_instance):
        """Add tagging for a task_instance."""
        dag_run = task_instance.dag_run
        # See TaskInstance definition, the "task" attribute may not be set
        task = getattr(task_instance, "task")
    
>       with sentry_sdk.configure_scope() as scope:
E       AttributeError: __enter__

airflow/sentry/configured.py:108: AttributeError
--------------------------------------------------------------------- Captured stdout setup ----------------------------------------------------------------------
========================= AIRFLOW ==========================
Home of the user: /root
Airflow home /root/airflow
Skipping initializing of the DB as it was initialized already.
You can re-initialize the database by adding --with-db-init flag when running tests.
======================================================================== warnings summary ========================================================================
tests/test_sentry.py::test_configured_sentry_add_tagging
  /usr/local/lib/python3.8/site-packages/flask_appbuilder/models/sqla/__init__.py:105: MovedIn20Warning: Deprecated API features detected! These feature(s) are not compatible with SQLAlchemy 2.0. To prevent incompatible upgrades prior to updating applications, ensure requirements files are pinned to "sqlalchemy<2.0". Set environment variable SQLALCHEMY_WARN_20=1 to show all deprecation warnings.  Set environment variable SQLALCHEMY_SILENCE_UBER_WARNING=1 to silence this message. (Background on SQLAlchemy 2.0 at: https://sqlalche.me/e/b8d9)
    @as_declarative(name="Model", metaclass=ModelDeclarativeMeta)

tests/test_sentry.py::test_configured_sentry_add_tagging
  /usr/local/lib/python3.8/site-packages/marshmallow_sqlalchemy/convert.py:17: DeprecationWarning: The '__version__' attribute is deprecated and will be removed in in a future version. Use feature detection or 'importlib.metadata.version("marshmallow")' instead.
    _META_KWARGS_DEPRECATED = Version(ma.__version__) >= Version("3.10.0")

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
==================================================================== short test summary info =====================================================================
FAILED tests/test_sentry.py::test_configured_sentry_add_tagging - AttributeError: __enter__
================================================================= 1 failed, 2 warnings in 4.77s ==================================================================

So this make me think that this could be either wrong implemented test or implementation do not work

cc: @LipuFei


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
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.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed @LipuFei - I missed that the test did not run and were placed at top level (that reminds me that we should prevent this).

@potiuk
Copy link
Member

potiuk commented Mar 26, 2024

You will have to redo that one @LipuFei and fix the problems @Taragolis noticed.

@potiuk potiuk merged commit a1473c9 into apache:main Mar 26, 2024
46 checks passed
@LipuFei
Copy link
Contributor

LipuFei commented Mar 26, 2024

No problem. @potiuk @Taragolis Thanks for the review. I will work on it later.

@potiuk
Copy link
Member

potiuk commented Mar 26, 2024

#38520 added to prevent similar cases.

@Taragolis Taragolis deleted the revert-37143 branch March 26, 2024 23:56
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Mar 27, 2024
mathiaHT pushed a commit to mathiaHT/airflow that referenced this pull request Apr 4, 2024
utkarsharma2 pushed a commit to astronomer/airflow that referenced this pull request Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants