Copilot commented on code in PR #6336: URL: https://github.com/apache/texera/pull/6336#discussion_r3563675897
########## .github/scripts/test_smoke_boot.sh: ########## @@ -0,0 +1,70 @@ +#!/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. + +# Regression test for smoke-boot.sh's crash-detection regex. Pulls `crash_re` +# straight out of smoke-boot.sh (single source of truth -- no duplicated pattern +# that could drift) and checks it against fixture boot logs. +# +# Guards issue #6332: jOOQ prints a random "tip of the day" banner naming +# NoClassDefFoundError / ClassNotFoundException in prose, which must NOT read as +# a boot crash -- while real thrown linkage errors still must. + +set -uo pipefail + +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# Load only the crash_re assignment line from smoke-boot.sh. Use command +# substitution (not `source <(...)`, which races the feeder process) so grep +# fully completes before eval runs. Input is our own controlled file. +eval "$(grep -E '^crash_re=' "$script_dir/smoke-boot.sh")" + +if [[ -z "${crash_re:-}" ]]; then + echo "FAIL: could not read crash_re from smoke-boot.sh" >&2 + exit 1 +fi + +rc=0 +# assert_no_crash <description> (boot log on stdin) +assert_no_crash() { + if grep -qE "$crash_re"; then echo "FAIL: $1 -- matched crash_re"; rc=1; else echo "ok: $1"; fi +} +# assert_crash <description> (boot log on stdin) +assert_crash() { + if grep -qE "$crash_re"; then echo "ok: $1"; else echo "FAIL: $1 -- did not match crash_re"; rc=1; fi +} + +# --- informational prose must NOT be treated as a crash (no random failures) --- +printf '%s\n' "jOOQ tip of the day: A NoClassDefFoundError or ClassNotFoundException is often a sign that your jOOQ code is generated with a different version of jOOQ than runtime library you're using" \ + | assert_no_crash "jOOQ tip of the day (#6332)" +printf '%s\n' 'INFO org.eclipse.jetty.server.Server: jetty-11.0.20 started' \ + | assert_no_crash "clean boot log" +printf '%s\n' 'DEBUG a NoSuchMethodError can occur when APIs drift' \ + | assert_no_crash "bare linkage-name mention without a fully-qualified type" + +# --- real thrown linkage failures must still be caught --- +printf '%s\n' 'Exception in thread "main" java.lang.NoClassDefFoundError: com/fasterxml/jackson/databind/ObjectMapper' \ + | assert_crash "thrown java.lang.NoClassDefFoundError" +printf '%s\n' 'Caused by: java.lang.ClassNotFoundException: org.apache.hadoop.fs.FileSystem' \ + | assert_crash "Caused by java.lang.ClassNotFoundException" +printf '%s\n' 'com.fasterxml.jackson.module.scala.JsonScalaEnumeration requires Jackson Databind version >= 2.15 but found 2.14' \ + | assert_crash "Jackson Databind version conflict (#6206)" Review Comment: The assertions are piped into `assert_no_crash` / `assert_crash`. In bash, functions invoked as part of a pipeline run in a subshell, so updates to `rc` inside the assertion functions won’t propagate back to the parent shell. This can make the test report failures in stdout but still exit 0, causing CI to miss regressions. ########## .github/scripts/smoke-boot.sh: ########## @@ -56,8 +56,13 @@ echo "smoke-boot: launching '$launcher' (port=$port timeout=${timeout}s)" pid=$! # Runtime classpath / linkage / module failures -- the class of regression this -# check exists to catch. -crash_re='NoClassDefFoundError|ClassNotFoundException|LinkageError|NoSuchMethodError|AbstractMethodError|ExceptionInInitializerError|IncompatibleClassChangeError|requires Jackson Databind' +# check exists to catch. Match the exception as an actual *thrown* type -- a +# fully-qualified java.lang.* name, or an "Exception in thread" header -- not the +# bare class name, which also shows up in harmless informational log prose (e.g. +# jOOQ prints a random "tip of the day" banner naming NoClassDefFoundError / +# ClassNotFoundException). Regression test: .github/scripts/test_smoke_boot.sh. +# See https://github.com/apache/texera/issues/6332. +crash_re='java\.lang\.(NoClassDefFoundError|ClassNotFoundException|LinkageError|NoSuchMethodError|AbstractMethodError|ExceptionInInitializerError|IncompatibleClassChangeError)|Exception in thread|requires Jackson Databind' Review Comment: The PR description says the crash detection was extracted into a sourceable `log_has_linkage_crash` with a guarded `main()`, but `smoke-boot.sh` is still a straight script that executes immediately and requires positional args. Either update the PR description, or refactor the script as described so tests can `source` it without relying on `eval "$(grep ...)"`. -- 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]
