Copilot commented on code in PR #61869: URL: https://github.com/apache/doris/pull/61869#discussion_r3019312082
########## regression-test/pipeline/common/stage-timer.sh: ########## @@ -0,0 +1,129 @@ +#!/usr/bin/env bash +# 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. + +stage_timer__now() { + date +%s +} + +stage_timer__format_seconds() { + local total_seconds="${1:-0}" + local hours=$((total_seconds / 3600)) + local minutes=$(((total_seconds % 3600) / 60)) + local seconds=$((total_seconds % 60)) + printf '%02d:%02d:%02d' "${hours}" "${minutes}" "${seconds}" +} + +stage_timer__extract_exit_trap() { + local trap_desc + trap_desc="$(trap -p EXIT)" + if [[ "${trap_desc}" =~ ^trap\ --\ \'(.*)\'\ EXIT$ ]]; then + printf '%s' "${BASH_REMATCH[1]}" + fi +} + +stage_timer__record_current_stage() { + local end_at="$1" + if [[ -z "${STAGE_TIMER_CURRENT_STAGE:-}" ]]; then + return 0 + fi + STAGE_TIMER_STAGE_NAMES+=("${STAGE_TIMER_CURRENT_STAGE}") + STAGE_TIMER_STAGE_SECONDS+=("$((end_at - STAGE_TIMER_CURRENT_STAGE_STARTED_AT))") + STAGE_TIMER_CURRENT_STAGE='' + STAGE_TIMER_CURRENT_STAGE_STARTED_AT='' +} + +stage_timer__print_summary() { + local exit_code="$1" + local finished_at="$2" + local total_seconds="$((finished_at - STAGE_TIMER_STARTED_AT))" + local index + + echo "========== ${STAGE_TIMER_PIPELINE_NAME} 阶段耗时汇总 ==========" + for index in "${!STAGE_TIMER_STAGE_NAMES[@]}"; do + printf '[stage-timer] %s: %s (%ss)\n' \ + "${STAGE_TIMER_STAGE_NAMES[$index]}" \ + "$(stage_timer__format_seconds "${STAGE_TIMER_STAGE_SECONDS[$index]}")" \ + "${STAGE_TIMER_STAGE_SECONDS[$index]}" + done + printf '[stage-timer] 总耗时: %s (%ss)\n' \ + "$(stage_timer__format_seconds "${total_seconds}")" \ + "${total_seconds}" + printf '[stage-timer] 退出码: %s\n' "${exit_code}" + echo "==================================================" +} + +stage_timer_finish() { + local exit_code="${1:-$?}" + local finished_at + if [[ "${STAGE_TIMER_INITIALIZED:-false}" != "true" ]]; then + return 0 + fi + if [[ "${STAGE_TIMER_SUMMARY_PRINTED:-false}" == "true" ]]; then + return 0 + fi + finished_at="$(stage_timer__now)" + stage_timer__record_current_stage "${finished_at}" + stage_timer__print_summary "${exit_code}" "${finished_at}" + STAGE_TIMER_SUMMARY_PRINTED=true +} + +stage_timer__handle_exit() { + local exit_code="${1:-0}" + stage_timer_finish "${exit_code}" + if [[ -n "${STAGE_TIMER_PREVIOUS_EXIT_TRAP:-}" ]] && + [[ "${STAGE_TIMER_PREVIOUS_EXIT_TRAP}" != *"stage_timer__handle_exit"* ]]; then Review Comment: In the EXIT trap chaining, the previous EXIT trap will observe an incorrect `$?` value because `stage_timer_finish` runs before `eval`ing the captured trap. If the previous trap uses `$?` (common for cleanup/reporting), it will see the status of `stage_timer_finish` (likely 0) instead of the script’s real exit code. Consider restoring `$?` to `exit_code` (e.g., via a no-op subshell ` (exit "$exit_code")` before `eval`) or otherwise ensuring the previous trap runs with the original exit status. ```suggestion [[ "${STAGE_TIMER_PREVIOUS_EXIT_TRAP}" != *"stage_timer__handle_exit"* ]]; then # Restore the original exit status so that any `$?` in the previous # EXIT trap observes the script's real exit code instead of the # status of stage_timer_finish. ( exit "${exit_code}" ) ``` ########## regression-test/pipeline/common/test_stage_timer.py: ########## @@ -0,0 +1,149 @@ +#!/usr/bin/env python Review Comment: This test script uses `tempfile.TemporaryDirectory()` which requires Python 3, but the shebang is `#!/usr/bin/env python` (may resolve to Python 2 on some environments). Update the shebang to `python3` (or remove it if the test is always invoked via an explicit interpreter) to avoid running under Python 2 and failing at import/runtime. ```suggestion #!/usr/bin/env python3 ``` ########## regression-test/pipeline/external/external-stage-timer.sh: ########## @@ -0,0 +1,119 @@ +#!/usr/bin/env bash +# 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. + +if [[ -z "${teamcity_build_checkoutDir:-}" ]]; then + echo "ERROR: env teamcity_build_checkoutDir not set" + return 1 2>/dev/null || exit 1 +fi + +# shellcheck source=/dev/null +source "${teamcity_build_checkoutDir}"/regression-test/pipeline/common/stage-timer.sh + +external_regression_stage_timer__extract_debug_trap() { + local trap_desc + trap_desc="$(trap -p DEBUG)" + if [[ "${trap_desc}" =~ ^trap\ --\ \'(.*)\'\ DEBUG$ ]]; then + printf '%s' "${BASH_REMATCH[1]}" + fi +} + +external_regression_stage_timer__enter_if_needed() { + local stage_name="${1:-}" + if [[ -z "${stage_name}" ]]; then + return 1 + fi + if [[ "${STAGE_TIMER_CURRENT_STAGE:-}" == "${stage_name}" ]]; then + return 0 + fi + stage_timer_enter "${stage_name}" +} + +external_regression_stage_timer__handle_command() { + local current_command="${1:-}" + if [[ -z "${current_command}" ]]; then + return 0 + fi + + case "${current_command}" in + *"bash deploy_cluster.sh "*) + external_regression_stage_timer__enter_if_needed "启动 Doris" + ;; + *"START EXTERNAL DOCKER"* | *"run-thirdparties-docker.sh "*) + if [[ "${current_command}" != *"--stop"* ]]; then + external_regression_stage_timer__enter_if_needed "启动依赖" + fi + ;; + *"./run-regression-test.sh --teamcity --clean --run"*) + external_regression_stage_timer__enter_if_needed "执行 Case" + ;; + *"COLLECT DOCKER LOGS"* | *"collect_docker_logs "*) + external_regression_stage_timer__enter_if_needed "收尾归档" + ;; + esac +} + +external_regression_stage_timer__run_previous_debug_trap() { + if [[ -n "${EXTERNAL_REGRESSION_STAGE_TIMER_PREVIOUS_DEBUG_TRAP:-}" ]] && + [[ "${EXTERNAL_REGRESSION_STAGE_TIMER_PREVIOUS_DEBUG_TRAP}" != *"external_regression_stage_timer__debug_hook"* ]]; then + eval "${EXTERNAL_REGRESSION_STAGE_TIMER_PREVIOUS_DEBUG_TRAP}" + fi +} + +external_regression_stage_timer__debug_hook() { + local current_command="${1:-$BASH_COMMAND}" + if [[ "${EXTERNAL_REGRESSION_STAGE_TIMER_IN_DEBUG_HOOK:-false}" == "true" ]]; then + return 0 + fi + + EXTERNAL_REGRESSION_STAGE_TIMER_IN_DEBUG_HOOK=true + external_regression_stage_timer__handle_command "${current_command}" + external_regression_stage_timer__run_previous_debug_trap + EXTERNAL_REGRESSION_STAGE_TIMER_IN_DEBUG_HOOK=false Review Comment: The DEBUG-trap chaining changes `$?` before running the previous DEBUG trap: `external_regression_stage_timer__handle_command` and other helper calls run first, so the previous trap will see their status (typically 0) rather than the original `$?` value from when the DEBUG trap fired. If an existing DEBUG trap depends on `$?`, this breaks behavior. Capture the incoming `$?` at the start of `external_regression_stage_timer__debug_hook` and restore it immediately before `eval`ing `EXTERNAL_REGRESSION_STAGE_TIMER_PREVIOUS_DEBUG_TRAP` (e.g., using `(exit "$saved_status")` as the immediately preceding command). -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
