You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2022/06/27 21:39:33 UTC
[GitHub] [trafficserver] bneradt opened a new pull request, #8928: proxy_serve_stale: Test updates
bneradt opened a new pull request, #8928:
URL: https://github.com/apache/trafficserver/pull/8928
This updates the proxy_serve_stale test to run more quickly (about 15
seconds instead of about 2 minutes). While doing so, the test is also
converted to use Proxy Verifier.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall commented on a diff in pull request #8928: proxy_serve_stale: Test updates
Posted by GitBox <gi...@apache.org>.
randall commented on code in PR #8928:
URL: https://github.com/apache/trafficserver/pull/8928#discussion_r908617299
##########
tests/gold_tests/proxy_protocol/proxy_serve_stale.test.py:
##########
@@ -17,64 +17,57 @@
# See the License for the specific language governing permissions and
# limitations under the License.
+
+Test.testName = "proxy_serve_stale"
Test.ContinueOnFail = True
-# Set up hierarchical caching processes
-ts_child = Test.MakeATSProcess("ts_child")
-# Parent ATS process is not created to mock parent being "down"
-# but parent hostname is recognized in hostdb to match with child successfully
-ts_parent_hostname = "localhost:82"
-server = Test.MakeOriginServer("server")
-Test.testName = "STALE"
-# Request from client
-request_header = {"headers":
- "GET / HTTP/1.1\r\nHost: localhost\r\n\r\n",
- "timestamp": "1469733493.993",
- "body": ""}
-# Expected response from the origin server
-response_header = {"headers":
- "HTTP/1.1 200 OK\r\nConnection: close\r\nCache-Control: max-age=5,public\r\n\r\n",
- "timestamp": "1469733493.993",
- "body": "CACHED"}
+class ProxyServeStaleTest:
+ """Verify that stale content is served when the parent is down."""
+
+ single_transaction_replay = "replay/proxy_serve_stale.replay.yaml"
+ ts_parent_hostname = "localhost:82"
+
+ def __init__(self):
+ """Initialize the test."""
+ self._configure_server()
+ self._configure_ts()
-# Add request/response
-server.addResponse("sessionlog.log", request_header, response_header)
+ def _configure_server(self):
+ self.server = Test.MakeVerifierServerProcess(
+ "server",
+ self.single_transaction_replay)
-# Config child proxy to route to parent proxy
-ts_child.Disk.records_config.update({
- 'proxy.config.http.parent_proxy.fail_threshold': 2,
- 'proxy.config.http.parent_proxy.total_connect_attempts': 1,
- 'proxy.config.http.cache.max_stale_age': 90,
- 'proxy.config.http.parent_proxy.self_detect': 0,
-})
-ts_child.Disk.parent_config.AddLine(
- f'dest_domain=. parent="{ts_parent_hostname}" round_robin=consistent_hash go_direct=false'
-)
-ts_child.Disk.remap_config.AddLine(
- f'map http://localhost:{ts_child.Variables.port} http://localhost:{server.Variables.Port}'
-)
+ def _configure_ts(self):
+ self.ts_child = Test.MakeATSProcess("ts_child")
+ # Config child proxy to route to parent proxy
+ self.ts_child.Disk.records_config.update({
+ 'proxy.config.http.parent_proxy.fail_threshold': 2,
+ 'proxy.config.http.parent_proxy.total_connect_attempts': 1,
+ 'proxy.config.http.cache.max_stale_age': 10,
+ 'proxy.config.http.parent_proxy.self_detect': 0,
+ 'proxy.config.diags.debug.enabled': 1,
Review Comment:
Just curious, are the debug tags required for proxy verifier?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #8928: proxy_serve_stale: Test updates
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #8928:
URL: https://github.com/apache/trafficserver/pull/8928#issuecomment-1168921437
> Looks good to me.
Thank you for the review, @serrislew.
> Is this the standard of how autests should be created now?
Standard is a strong word. In case it's helpful to share, here are a couple of my observations with the AuTests:
### Test Class Organization
I noticed when reviewing @masaori335's PRs that when he adds AuTests he structures them around a test class with helper methods that define each component of the test (setup_traffic_server, setup_server, run_the_cases, etc.). I find this a very helpful way to organize the test for readability. So I picked this up from him and generally follow his example.
### Proxy Verifier
Proxy Verifier has some advantages over curl:
* It has an expressive traffic verification mechanism that is inlined with the traffic description (the replay files). This is a more purpose built and explicit method for verifying the traffic than using gold files against curl output. Gold files are fine in many contexts (for verifying log output, for instance), but the gold matching mechanism is a pretty generic way to verify output in comparison to being able to verify the characteristics of a header with various directives (`equal`, `contains`, `present`, case sensitive or insensitive, etc), or a response code, or the HTTP body, or any combination of the three.
* Along these lines, Proxy Verifier test failures in AuTest will print out the specific Verifier error message indicating what went wrong. For example, an error message might say, `[ERROR]: HTTP/1 Status Violation: expected 200 got 201, key: push`. This is usually a more precise and readable AuTest failure than a gold file diff output.
* Helpful for this test, it also has delay mechanisms that can be expressed in the replay file.
* With curl, if a server is needed, the client (curl) and the server (usually microserver) have to be configured independently of each other. With Proxy Verifier, the same replay file configures both the client and the server.
* When using multiple curl calls in AuTest, they are often separated by independent TestRun objects. (That wasn't the case with this test, but often separate runs are helpful for verifying each curl result independently.) AuTest has some overhead when processing the processes of each test run which slows down the test. When all the transactions are specified in a single TestRun invocation of Proxy Verifier client and server, this can greatly speed up the test.
For these reasons, I prefer using Proxy Verifier over curl in many cases. But I'm biased since I maintain the Proxy Verifier project (@SolidWallOfCode wrote the initial version of Proxy Verifier, I now maintain it). That said, I still do sometimes use curl when all that is needed is a simple request to exercise ATS when no server is needed. In those cases, creating a replay file is verbose in comparison to a simple curl command.
---
That guides me when I work with AuTests. But I won't block PRs that don't follow these practices.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt merged pull request #8928: proxy_serve_stale: Test updates
Posted by GitBox <gi...@apache.org>.
bneradt merged PR #8928:
URL: https://github.com/apache/trafficserver/pull/8928
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #8928: proxy_serve_stale: Test updates
Posted by GitBox <gi...@apache.org>.
zwoop commented on PR #8928:
URL: https://github.com/apache/trafficserver/pull/8928#issuecomment-1176458956
Cherry-picked to v9.2.x
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #8928: proxy_serve_stale: Test updates
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #8928:
URL: https://github.com/apache/trafficserver/pull/8928#discussion_r908623791
##########
tests/gold_tests/proxy_protocol/proxy_serve_stale.test.py:
##########
@@ -17,64 +17,57 @@
# See the License for the specific language governing permissions and
# limitations under the License.
+
+Test.testName = "proxy_serve_stale"
Test.ContinueOnFail = True
-# Set up hierarchical caching processes
-ts_child = Test.MakeATSProcess("ts_child")
-# Parent ATS process is not created to mock parent being "down"
-# but parent hostname is recognized in hostdb to match with child successfully
-ts_parent_hostname = "localhost:82"
-server = Test.MakeOriginServer("server")
-Test.testName = "STALE"
-# Request from client
-request_header = {"headers":
- "GET / HTTP/1.1\r\nHost: localhost\r\n\r\n",
- "timestamp": "1469733493.993",
- "body": ""}
-# Expected response from the origin server
-response_header = {"headers":
- "HTTP/1.1 200 OK\r\nConnection: close\r\nCache-Control: max-age=5,public\r\n\r\n",
- "timestamp": "1469733493.993",
- "body": "CACHED"}
+class ProxyServeStaleTest:
+ """Verify that stale content is served when the parent is down."""
+
+ single_transaction_replay = "replay/proxy_serve_stale.replay.yaml"
+ ts_parent_hostname = "localhost:82"
+
+ def __init__(self):
+ """Initialize the test."""
+ self._configure_server()
+ self._configure_ts()
-# Add request/response
-server.addResponse("sessionlog.log", request_header, response_header)
+ def _configure_server(self):
+ self.server = Test.MakeVerifierServerProcess(
+ "server",
+ self.single_transaction_replay)
-# Config child proxy to route to parent proxy
-ts_child.Disk.records_config.update({
- 'proxy.config.http.parent_proxy.fail_threshold': 2,
- 'proxy.config.http.parent_proxy.total_connect_attempts': 1,
- 'proxy.config.http.cache.max_stale_age': 90,
- 'proxy.config.http.parent_proxy.self_detect': 0,
-})
-ts_child.Disk.parent_config.AddLine(
- f'dest_domain=. parent="{ts_parent_hostname}" round_robin=consistent_hash go_direct=false'
-)
-ts_child.Disk.remap_config.AddLine(
- f'map http://localhost:{ts_child.Variables.port} http://localhost:{server.Variables.Port}'
-)
+ def _configure_ts(self):
+ self.ts_child = Test.MakeATSProcess("ts_child")
+ # Config child proxy to route to parent proxy
+ self.ts_child.Disk.records_config.update({
+ 'proxy.config.http.parent_proxy.fail_threshold': 2,
+ 'proxy.config.http.parent_proxy.total_connect_attempts': 1,
+ 'proxy.config.http.cache.max_stale_age': 10,
+ 'proxy.config.http.parent_proxy.self_detect': 0,
+ 'proxy.config.diags.debug.enabled': 1,
Review Comment:
Thank you for the review.
> Just curious, are the debug tags required for proxy verifier?
No, they are not needed by Proxy Verifier nor are they otherwise needed by the test to function correctly.
However I generally include the debug tags in AuTests that are relevant to the features being tested as a convenience for someone who later wants to follow ATS's logic later. It's maybe most helpful when analyzing a CI sandbox after a test fails, for instance. In that case, it can be hard to go back and add the tag in CI to find out why the test ATS Process is behaving the way it is, maybe behaving that way only in CI.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall commented on a diff in pull request #8928: proxy_serve_stale: Test updates
Posted by GitBox <gi...@apache.org>.
randall commented on code in PR #8928:
URL: https://github.com/apache/trafficserver/pull/8928#discussion_r908733309
##########
tests/gold_tests/proxy_protocol/proxy_serve_stale.test.py:
##########
@@ -17,64 +17,57 @@
# See the License for the specific language governing permissions and
# limitations under the License.
+
+Test.testName = "proxy_serve_stale"
Test.ContinueOnFail = True
-# Set up hierarchical caching processes
-ts_child = Test.MakeATSProcess("ts_child")
-# Parent ATS process is not created to mock parent being "down"
-# but parent hostname is recognized in hostdb to match with child successfully
-ts_parent_hostname = "localhost:82"
-server = Test.MakeOriginServer("server")
-Test.testName = "STALE"
-# Request from client
-request_header = {"headers":
- "GET / HTTP/1.1\r\nHost: localhost\r\n\r\n",
- "timestamp": "1469733493.993",
- "body": ""}
-# Expected response from the origin server
-response_header = {"headers":
- "HTTP/1.1 200 OK\r\nConnection: close\r\nCache-Control: max-age=5,public\r\n\r\n",
- "timestamp": "1469733493.993",
- "body": "CACHED"}
+class ProxyServeStaleTest:
+ """Verify that stale content is served when the parent is down."""
+
+ single_transaction_replay = "replay/proxy_serve_stale.replay.yaml"
+ ts_parent_hostname = "localhost:82"
+
+ def __init__(self):
+ """Initialize the test."""
+ self._configure_server()
+ self._configure_ts()
-# Add request/response
-server.addResponse("sessionlog.log", request_header, response_header)
+ def _configure_server(self):
+ self.server = Test.MakeVerifierServerProcess(
+ "server",
+ self.single_transaction_replay)
-# Config child proxy to route to parent proxy
-ts_child.Disk.records_config.update({
- 'proxy.config.http.parent_proxy.fail_threshold': 2,
- 'proxy.config.http.parent_proxy.total_connect_attempts': 1,
- 'proxy.config.http.cache.max_stale_age': 90,
- 'proxy.config.http.parent_proxy.self_detect': 0,
-})
-ts_child.Disk.parent_config.AddLine(
- f'dest_domain=. parent="{ts_parent_hostname}" round_robin=consistent_hash go_direct=false'
-)
-ts_child.Disk.remap_config.AddLine(
- f'map http://localhost:{ts_child.Variables.port} http://localhost:{server.Variables.Port}'
-)
+ def _configure_ts(self):
+ self.ts_child = Test.MakeATSProcess("ts_child")
+ # Config child proxy to route to parent proxy
+ self.ts_child.Disk.records_config.update({
+ 'proxy.config.http.parent_proxy.fail_threshold': 2,
+ 'proxy.config.http.parent_proxy.total_connect_attempts': 1,
+ 'proxy.config.http.cache.max_stale_age': 10,
+ 'proxy.config.http.parent_proxy.self_detect': 0,
+ 'proxy.config.diags.debug.enabled': 1,
Review Comment:
Thanks for the explanation!
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #8928: proxy_serve_stale: Test updates
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #8928:
URL: https://github.com/apache/trafficserver/pull/8928#issuecomment-1167935976
@serrislew: can you please review this?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] serrislew commented on pull request #8928: proxy_serve_stale: Test updates
Posted by GitBox <gi...@apache.org>.
serrislew commented on PR #8928:
URL: https://github.com/apache/trafficserver/pull/8928#issuecomment-1168036525
Looks good to me. Is this the standard of how autests should be created now?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org