From 5ddea65a0cb60d810884f9065d45df1f0b94f133 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Sun, 4 Dec 2022 17:24:59 +0100 Subject: [PATCH] Improve handling of warnings in CI Warnings printed in CI have been making it difficult to see what is going on (they were taking far too much space after the test results and GitHub CI UI rendered those multi-line warnings slowly. Also we did not have the right tools to capture the number and list of warnings that we should deal with. We are usign pytest-capture-warnings plugin now that improves the situation twofold: * warning summary printed by the plugin in the output is shorter - each warning is at most one line * the warning text files are uploaded as artifacts which make them usable in any kind of approach where we want to attempt to start an effort to remove all warnings --- .github/actions/post_tests/action.yml | 6 ++++ .gitignore | 2 +- .rat-excludes | 3 ++ Dockerfile.ci | 3 ++ TESTING.rst | 3 ++ scripts/docker/entrypoint_ci.sh | 3 ++ scripts/in_container/filter_out_warnings.py | 31 +++++++++++++++++++ scripts/in_container/run_ci_tests.sh | 8 +++-- setup.py | 1 + .../slack/hooks/test_slack_webhook.py | 6 ++-- 10 files changed, 60 insertions(+), 6 deletions(-) create mode 100644 scripts/in_container/filter_out_warnings.py diff --git a/.github/actions/post_tests/action.yml b/.github/actions/post_tests/action.yml index 2e15b01ba8bdb..96bed9211bb5b 100644 --- a/.github/actions/post_tests/action.yml +++ b/.github/actions/post_tests/action.yml @@ -42,6 +42,12 @@ runs: name: coverage-${{env.JOB_ID}} path: ./files/coverage*.xml retention-days: 7 + - name: "Upload artifact for warnings" + uses: actions/upload-artifact@v3 + with: + name: test-warnings-${{env.JOB_ID}} + path: ./files/warnings-*.txt + retention-days: 7 - name: "Fix ownership" shell: bash run: breeze ci fix-ownership diff --git a/.gitignore b/.gitignore index 98c9dc2b239b2..edd1362f96c3e 100644 --- a/.gitignore +++ b/.gitignore @@ -14,10 +14,10 @@ unittests.db airflow/git_version airflow/www/static/coverage/ airflow/www/*.log - /logs/ airflow-webserver.pid standalone_admin_password.txt +warnings.txt # Byte-compiled / optimized / DLL files __pycache__/ diff --git a/.rat-excludes b/.rat-excludes index 56c81748371c5..1e16d61a67f5d 100644 --- a/.rat-excludes +++ b/.rat-excludes @@ -121,6 +121,9 @@ chart/values_schema.schema.json # Newsfragments are snippets that will be, eventually, consumed into RELEASE_NOTES newsfragments/* +# Warning file generated +warnings.txt + # Dev stuff tests/* scripts/* diff --git a/Dockerfile.ci b/Dockerfile.ci index 149bfc17f44d6..f2e64830fbbd6 100644 --- a/Dockerfile.ci +++ b/Dockerfile.ci @@ -802,6 +802,7 @@ fi set -u export RESULT_LOG_FILE="/files/test_result-${TEST_TYPE/\[*\]/}-${BACKEND}.xml" +export WARNINGS_FILE="/files/warnings-${TEST_TYPE/\[*\]/}-${BACKEND}.txt" EXTRA_PYTEST_ARGS=( "--verbosity=0" @@ -816,6 +817,8 @@ EXTRA_PYTEST_ARGS=( "--setup-timeout=${TEST_TIMEOUT}" "--execution-timeout=${TEST_TIMEOUT}" "--teardown-timeout=${TEST_TIMEOUT}" + "--output=${WARNINGS_FILE}" + "--disable-warnings" # Only display summary for non-expected case # f - failed # E - error diff --git a/TESTING.rst b/TESTING.rst index d47adc6ad11da..b0fc8be512e7c 100644 --- a/TESTING.rst +++ b/TESTING.rst @@ -51,6 +51,9 @@ Follow the guidelines when writing unit tests: * For new tests, use standard "asserts" of Python and ``pytest`` decorators/context managers for testing rather than ``unittest`` ones. See `pytest docs `_ for details. * Use a parameterized framework for tests that have variations in parameters. +* Use with ``pytest.warn`` to capture warnings rather than ``recwarn`` fixture. We are aiming for 0-warning in our + tests, so we run Pytest with ``--disable-warnings`` but instead we have ``pytest-capture-warnings`` plugin that + overrides ``recwarn`` fixture behaviour. **NOTE:** We plan to convert all unit tests to standard "asserts" semi-automatically, but this will be done later in Airflow 2.0 development phase. That will include setUp/tearDown/context managers and decorators. diff --git a/scripts/docker/entrypoint_ci.sh b/scripts/docker/entrypoint_ci.sh index 87bce601160b1..ca147c61c031d 100755 --- a/scripts/docker/entrypoint_ci.sh +++ b/scripts/docker/entrypoint_ci.sh @@ -245,6 +245,7 @@ fi set -u export RESULT_LOG_FILE="/files/test_result-${TEST_TYPE/\[*\]/}-${BACKEND}.xml" +export WARNINGS_FILE="/files/warnings-${TEST_TYPE/\[*\]/}-${BACKEND}.txt" EXTRA_PYTEST_ARGS=( "--verbosity=0" @@ -259,6 +260,8 @@ EXTRA_PYTEST_ARGS=( "--setup-timeout=${TEST_TIMEOUT}" "--execution-timeout=${TEST_TIMEOUT}" "--teardown-timeout=${TEST_TIMEOUT}" + "--output=${WARNINGS_FILE}" + "--disable-warnings" # Only display summary for non-expected case # f - failed # E - error diff --git a/scripts/in_container/filter_out_warnings.py b/scripts/in_container/filter_out_warnings.py new file mode 100644 index 0000000000000..df27eda7e6876 --- /dev/null +++ b/scripts/in_container/filter_out_warnings.py @@ -0,0 +1,31 @@ +#!/usr/bin/env python +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from __future__ import annotations + +import fileinput + +suppress = False + +for line in fileinput.input(): + if line.startswith("warnings summary:"): + suppress = True + if line.startswith("All Warning errors can be found in"): + suppress = False + if not suppress: + print(line, end="") diff --git a/scripts/in_container/run_ci_tests.sh b/scripts/in_container/run_ci_tests.sh index efab241fb7711..963375d77645d 100755 --- a/scripts/in_container/run_ci_tests.sh +++ b/scripts/in_container/run_ci_tests.sh @@ -23,10 +23,14 @@ echo "Starting the tests with those pytest arguments:" "${@}" echo set +e -pytest "${@}" - +pytest "${@}" | python "$( dirname "${BASH_SOURCE[0]}" )/filter_out_warnings.py" RES=$? +if [[ -f ${WARNINGS_FILE} ]]; then + echo "Number of warnings: $(wc -l "${WARNINGS_FILE}")" +fi + + if [[ ${RES} == "139" ]]; then echo "${COLOR_YELLOW}Sometimes Pytest fails at exiting with segfault, but all tests actually passed${COLOR_RESET}" echo "${COLOR_YELLOW}We should ignore such case. Checking if junitxml file ${RESULT_LOG_FILE} is there with 0 errors and failures${COLOR_RESET}" diff --git a/setup.py b/setup.py index 78d501f49510b..f5625835ba88b 100644 --- a/setup.py +++ b/setup.py @@ -391,6 +391,7 @@ def write_version(filename: str = str(AIRFLOW_SOURCES_ROOT / "airflow" / "git_ve # TODO: upgrade it and remove the limit "pytest~=6.0", "pytest-asyncio", + "pytest-capture-warnings", "pytest-cov", "pytest-instafail", # We should attempt to remove the limit when we upgrade Pytest diff --git a/tests/providers/slack/hooks/test_slack_webhook.py b/tests/providers/slack/hooks/test_slack_webhook.py index f687675891e7d..0c3527902f86f 100644 --- a/tests/providers/slack/hooks/test_slack_webhook.py +++ b/tests/providers/slack/hooks/test_slack_webhook.py @@ -526,13 +526,13 @@ def test_hook_send_by_hook_attributes(self, mock_hook_send_dict, deprecated_hook mock_hook_send_dict.assert_called_once_with(body=expected_body, headers=None) @mock.patch("airflow.providers.slack.hooks.slack_webhook.WebhookClient") - def test_hook_ignored_attributes(self, mock_webhook_client_cls, recwarn): + def test_hook_ignored_attributes(self, mock_webhook_client_cls): """Test hook constructor warn users about ignored attributes.""" mock_webhook_client = mock_webhook_client_cls.return_value mock_webhook_client_send_dict = mock_webhook_client.send_dict mock_webhook_client_send_dict.return_value = MOCK_WEBHOOK_RESPONSE - - hook = SlackWebhookHook(slack_webhook_conn_id=TEST_CONN_ID, link_names="test-value") + with pytest.warns(UserWarning) as recwarn: + hook = SlackWebhookHook(slack_webhook_conn_id=TEST_CONN_ID, link_names="test-value") assert len(recwarn) == 2 assert str(recwarn.pop(UserWarning).message).startswith( "`link_names` has no affect, if you want to mention user see:"