Copilot commented on code in PR #13833:
URL: https://github.com/apache/trafficserver/pull/13833#discussion_r4260856211
##########
tests/gold_tests/h2/http2_rst_stream.test.py:
##########
@@ -18,12 +18,25 @@
# See the License for the specific language governing permissions and
# limitations under the License.
+import re
+
Test.Summary = '''
Abort HTTP/2 connection using RST_STREAM frame.
'''
Test.SkipUnless(Condition.HasOpenSSLVersion('1.1.1'),
Condition.HasProxyVerifierVersion('2.8.0'))
+
+# When the client resets its stream, the origin sees the request only if ATS
finished
+# its TLS handshake with the origin first. In that case the origin answers,
the response
+# has nowhere to go, and ATS resets the origin stream with NO_ERROR after the
origin has
+# already closed it. That is fine. The client's abort must never reset an
origin stream
+# that is still open, which the origin logs as an RST_STREAM followed by the
stream closing.
+def origin_stream_not_reset():
+ return Testers.ExcludesExpression(
Review Comment:
The function name reads like it returns a boolean predicate, but it actually
returns a `Testers.ExcludesExpression(...)` object. Renaming to something
expectation-oriented (e.g., `expect_origin_stream_not_reset` /
`origin_stream_not_reset_check`) would make call sites clearer and reduce
confusion when reading the test.
##########
tests/gold_tests/h2/http2_rst_stream.test.py:
##########
@@ -18,12 +18,25 @@
# See the License for the specific language governing permissions and
# limitations under the License.
+import re
+
Test.Summary = '''
Abort HTTP/2 connection using RST_STREAM frame.
'''
Test.SkipUnless(Condition.HasOpenSSLVersion('1.1.1'),
Condition.HasProxyVerifierVersion('2.8.0'))
+
+# When the client resets its stream, the origin sees the request only if ATS
finished
+# its TLS handshake with the origin first. In that case the origin answers,
the response
+# has nowhere to go, and ATS resets the origin stream with NO_ERROR after the
origin has
+# already closed it. That is fine. The client's abort must never reset an
origin stream
+# that is still open, which the origin logs as an RST_STREAM followed by the
stream closing.
+def origin_stream_not_reset():
+ return Testers.ExcludesExpression(
+ 'Received RST_STREAM frame.*HTTP/2 stream is closed', 'Server is not
affected.', reflags=re.DOTALL | re.MULTILINE)
Review Comment:
The regex is very broad with `DOTALL`, so it can match `Received RST_STREAM
frame` from one stream/connection and `HTTP/2 stream is closed` from another
later in the same log, which could reintroduce flakiness under parallelism or
additional log noise. Consider tightening the pattern to the specific stream
(e.g., include stream id/key if present in the origin logs) and/or make the
`.*` non-greedy (`.*?`) with additional surrounding context so the match only
occurs for the same event sequence you intend to flag.
--
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]
