Copilot commented on code in PR #14055: URL: https://github.com/apache/apisix/pull/14055#discussion_r4323396001
########## t/cli/test_proxy_cache_purge.sh: ########## @@ -0,0 +1,139 @@ +#!/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. +# + +. ./t/cli/common.sh Review Comment: This test relies on bare `[` assertions later in the script (e.g., `[ \"$(cache_status $url)\" = \"MISS\" ]`). Unless `./t/cli/common.sh` enables `errexit`, these assertions won’t fail the script, which can yield false positives. Make the failure behavior explicit in this script (e.g., enable errexit/pipefail near the top, or replace the bare assertions with `if` blocks that `exit 1` on mismatch). ########## t/cli/test_proxy_cache_purge.sh: ########## @@ -0,0 +1,139 @@ +#!/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. +# + +. ./t/cli/common.sh + +# The http block of the generated nginx.conf enables open_file_cache, which the +# test-nginx based cases don't have. A disk cache entry removed by PURGE must not +# be served again from a file descriptor cached before the purge. + +echo ' +deployment: + admin: + admin_key_required: false + admin_key: ~ +nginx_config: + worker_processes: 1 + http_configuration_snippet: | + server { + listen 1980; + location / { + return 200 "hello"; + } + } +' > conf/config.yaml + +rm -rf /tmp/disk_cache_one/* +make run +sleep 1 + +admin() { + curl -s -o /dev/null -w "%{http_code}" http://127.0.0.1:9180/apisix/admin$1 -X PUT -d "$2" +} Review Comment: The Admin API payload is JSON, but the request doesn’t set `Content-Type: application/json`. If the admin endpoint becomes stricter about content types, this test will start failing for reasons unrelated to the PURGE behavior. Consider adding the explicit JSON content-type header to the admin helper. ########## t/cli/test_proxy_cache_purge.sh: ########## @@ -0,0 +1,139 @@ +#!/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. +# + +. ./t/cli/common.sh + +# The http block of the generated nginx.conf enables open_file_cache, which the +# test-nginx based cases don't have. A disk cache entry removed by PURGE must not +# be served again from a file descriptor cached before the purge. + +echo ' +deployment: + admin: + admin_key_required: false + admin_key: ~ +nginx_config: + worker_processes: 1 + http_configuration_snippet: | + server { + listen 1980; + location / { + return 200 "hello"; + } + } +' > conf/config.yaml + +rm -rf /tmp/disk_cache_one/* +make run +sleep 1 Review Comment: Using a fixed `sleep 1` after `make run` can make the test flaky on slower CI machines (service may not be ready yet) or unnecessarily slow on fast machines. Prefer a readiness check (polling the Admin API or a health endpoint with a timeout) so the test deterministically waits until APISIX is accepting requests. -- 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]
