stoty commented on code in PR #239: URL: https://github.com/apache/calcite-avatica/pull/239#discussion_r1502483355
########## server/src/test/java/org/apache/calcite/avatica/server/HttpServerSpnegoWithoutJaasTest.java: ########## @@ -186,6 +187,14 @@ private static void setupUsers(File keytabDir) throws KrbException { assertEquals(401, conn.getResponseCode()); } + @Test public void testServerVersionNotReturnedForUnauthorisedAccess() throws Exception { Review Comment: The rationale is that the tests are supposed to be behavioural based, and not build on implementation details. We know that there is a single switch in Jetty, and it applies to every message that Jetty sends, so testing for one case is fine. However, in theory it is possible that the Jetty implementation changes in the future, and the behaviour wil not be the same for the different authentication methods, and the swich will work for SPENGO or BASIC authenticaton, but not the other one. If we want to test for this - very unlikely - possibility, then we need to add the test to every variant, i.e. every child of the _HttpAuthBase_ class. In reality, TDD has its limits, and we cannot write a test case for everything, neither do we have the resources to run them, so there is a lot of room for interpretation. The chances of Jetty deliberately changing the behaviour of the flag is very slim, but there is also a very slight chance of a future Jetty bug that would change the behaviour, which this test might catch. Having the test only in the most basic (no pun intended) BasicAuthHttpServerTest is also fine by me, choose whichever one you like. -- 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: commits-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org