David Knupp has posted comments on this change. ( http://gerrit.cloudera.org:8080/15752 )
Change subject: IMPALA-9540 Test that Impala Shell no longer sends duplicate "Host" headers in http mode. ...................................................................... Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/15752/1/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/15752/1/tests/shell/test_shell_interactive.py@88 PS1, Line 88: self.headers.headers > OK headers.headers does look weird. Sorry, it took me a while to reason through this. I have a couple of thoughts. This first is that both the TestRequestHandler and the test could probably be simplified if you moved the assert about the number of "Host:" headers into the handler itself. I'm not sure that storing the headers temporarily only to check them from some external context buys you much, but it makes the code harder to follow. So maybe something like this: # yeah, I changed the name class RequestHandler503(SimpleHTTPServer.SimpleHTTPRequestHandler): """A custom http handler that stores the headers from the most recent http request, and always returns a 503 http code""" def do_POST(self): # The unfortunately named self.headers here is an instance of mimetools.Message that # contains the request headers request_headers = self.headers.headers # Ensure that only one 'Host' header is contained in the request before responding host_hdr_count = sum([header.startswith('Host:') for header in request_headers]) assert host_hdr_count == 1, "duplicate 'Host:' headers in %s" % request_headers # respond with 503. self.send_response(code=httplib.SERVICE_UNAVAILABLE, message="Service Unavailable") The other thing that I thought about (and this is on me, as someone who reviewed the earlier incarnation of this test) is that you really could also clean up the test quite a bit if you used a fixture for the server. In general, if you find that you have a lot of boilerplate setup/teardown code in the body of a test that has nothing to do with the product being tested, it's a test code smell. (Neat aside that supports this -- if there's a failure in the setup or teardown within the body of the test, it gets flagged in junit_xml by pytest as a FAIL'ed test, i.e., product bug. If the setup/teardown fails from a fixture context, it's flagged as an ERROR, which usually indicates framework or environment issue. We tend to violate this distinction all over the place.) Anyway, another approach to further refactor could go so far as to write a fixture like this, using the class above. @pytest.yield_fixture def http_503_server(): class TestHTTPServer503(object): def __init__(self): self.HOST = "localhost" self.PORT = get_unused_port() self.httpd = SocketServer.TCPServer((self.HOST, self.PORT), RequestHandler503) self.http_server_thread = threading.Thread(target=self.httpd.serve_forever) self.http_server_thread.start() server = TestHTTPServer503() yield server # cleanup after test if server.httpd is not None: server.httpd.shutdown() if server.http_server_thread is not None: server.http_server_thread.join() Which would reduce the test to this... def test_http_interactions(self, vector, http_503_server): """Test interactions with the http server when using hs2-http protocol. Check that the shell prints a good message when the server returns a 503 error.""" protocol = vector.get_value("protocol") if protocol != 'hs2-http': pytest.skip() # Check that we get a message about the 503 error when we try to connect. shell_args = ["--protocol={0}".format(protocol), "-i{0}:{1}".format(http_503_server.HOST, http_503_server.PORT)] shell_proc = pexpect.spawn(IMPALA_SHELL_EXECUTABLE, shell_args) shell_proc.expect("HTTP code 503", timeout=10) That's a lot to throw at you. I'd frankly be happy if we could just clean up the handler class, if you agree with my recommendation. -- To view, visit http://gerrit.cloudera.org:8080/15752 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82996015d0205923e854dac8bb88604778684c46 Gerrit-Change-Number: 15752 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Thu, 30 Apr 2020 02:51:45 +0000 Gerrit-HasComments: Yes
