Re: [PR] Remove core imports in shared observability tests [airflow]
amoghrajesh merged PR #61784: URL: https://github.com/apache/airflow/pull/61784 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Remove core imports in shared observability tests [airflow]
amoghrajesh commented on PR #61784: URL: https://github.com/apache/airflow/pull/61784#issuecomment-3889018855 > Changes LGTM! If the failure persists, it might be worth launching a CI that runs the failing test on repeat. Nope, there are no actual failures on main, its when I tried to work on a related PR that I did notice this pattern which can be eliminated -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Remove core imports in shared observability tests [airflow]
amoghrajesh commented on code in PR #61784: URL: https://github.com/apache/airflow/pull/61784#discussion_r2797079525 ## shared/observability/tests/observability/metrics/test_stats.py: ## @@ -149,7 +148,8 @@ def test_load_invalid_custom_stats_client(self): "Your custom StatsD client must extend the statsd." "StatsClient in order to ensure backwards compatibility." ) -with pytest.raises(AirflowConfigException, match=error_message): +# we assert for Exception here instead of AirflowConfigException to not import from shared configuration +with pytest.raises(Exception, match=error_message): Review Comment: We would have to add `configuration` from `shared` as a dev dependency, which can be avoided by doing this. This kind of simulates that because `AirflowConfigException` inherits from `Exception` and we match by error message which is thrown by AirflowConfigException, so I think its good enough -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Remove core imports in shared observability tests [airflow]
amoghrajesh commented on PR #61784: URL: https://github.com/apache/airflow/pull/61784#issuecomment-3889012905 Good catch @andreahlert! I did that earlier and since I am using `Exception` class directly, I removed that -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Remove core imports in shared observability tests [airflow]
andreahlert commented on code in PR #61784: URL: https://github.com/apache/airflow/pull/61784#discussion_r2795196475 ## shared/observability/tests/observability/metrics/test_stats.py: ## @@ -149,7 +148,8 @@ def test_load_invalid_custom_stats_client(self): "Your custom StatsD client must extend the statsd." "StatsClient in order to ensure backwards compatibility." ) -with pytest.raises(AirflowConfigException, match=error_message): +# we assert for Exception here instead of AirflowConfigException to not import from shared configuration +with pytest.raises(Exception, match=error_message): Review Comment: would it work to import AirflowConfigException from airflow_shared.configuration here instead of catching bare Exception? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Remove core imports in shared observability tests [airflow]
andreahlert commented on PR #61784: URL: https://github.com/apache/airflow/pull/61784#issuecomment-3886716111 hey, the description mentions adding apache-airflow-shared-configuration as a dep but didn't see the pyproject.toml change. Was that dropped? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
