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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
