On 30/09/12 23:46, Brian Burch wrote:
On 30/09/12 21:56, Konstantin Kolinko wrote:
2012/9/26 Brian Burch <br...@pingtoo.com>:
Thanks for all the help while I was developing the new test case for
https://issues.apache.org/bugzilla/show_bug.cgi?id=53584. The thread
on the
users mailing list is called "AuthenticatorBase
setChangeSessionIdOnAuthentication without cookies".
I now have two new test cases working successfully against a
recently-updated trunk. I hope to use them in future as boilerplates, to
expand the set of tests, and also to examine SSO behaviour.
Before I open a bugzilla enhancement request and submit my patch
files, I
would like to discuss my implementation decisions in general, to
ensure that
my effort, and other developer's time, isn't wasted.
I found it necessary to modify both
org.apache.catalina.authenticator.FormAuthenticatorTest, and its parent
class org.apache.catalina.startup.SimpleHttpClient.
To save you looking it up, here is the unchanged class comment to
SimpleHttpClient:
/**
* Simple client for unit testing. It isn't robust, it isn't secure and
* should not be used as the basis for production code. Its only
purpose
* is to do the bare minimum for the unit tests.
*/
FormAuthenticatorTest doesn't have a class-level comment at all, but
I have
written a new one based on the conclusions in my thread on the users
list.
I would have preferred to make fairly localised changes to both of these
classes, but the existing logic seems to incorporate some fundamental
assumptions that made my intention too difficult to implement.
I am not at all proud of my code, but I believe it a) works, b) is
fairly
harmonious with the existing design, and c) is amenable to the
extensions I
intend to add in due course.
Firstly, I've written several small blocks of parser logic for urls
and http
headers, which are specific to the test environment and not very
pretty. I
looked for suitable parsers to re-use in the various tomcat utils
packages,
but haven't found them yet, even though tomcat MUST be doing similar
work in
an elegant and robust manner. I haven't found any unit tests, either.
Then, I looked at the apache HttpClient project. An ideal solution would
have been to use the parsers from that project, or perhaps even the
entire
client. This would have required starting with a blank sheet of
paper, and I
am very reluctant to take that approach. I might have been swayed if
I had
found HttpClient already in use by other tomcat unit tests, but I
couldn't
find it in the dependencies and didn't want to add a new one.
Next, the current version only supports cookies, so I had to add
some extra
boolean arguments to control the use of server and client cookies in
each
test case. In my professional work, I would have use singleton inner
classes
to achieve type-safety and make these crucial arguments
self-documenting,
but this wouldn't be compatible with the existing style of the various
current authenticator unit test classes. Also, I wanted to make my
initial
change as transparent as possible, so it could be reviewed (and
accepted)
with as little effort from others as possible.
I didn't want to touch SimpleHttpClient at all, but that turned out
to be
unavoidable. This class does most of the http header processing, and
so it
seemed ugly to split this work between the two classes. On the other
hand,
all the url handling is done by FormAuthenticatorTest, so it felt
ugly to
start doing any of that work in SimpleHttpClient. The consequence is
that
the two classes need to be cross-wired to some extent. This was
always the
case, but I had to cross some more wires for the new test cases.
So, to summarise: I would like to make quite a bulky change to these two
classes. I am somewhat embarrassed by my ugly style and DIY approach to
parsing. Many of the line-changes in the patch are trivial, but a lot of
lines will be hit at once. I haven't gone mad with comments, but have
tried
to add useful tips when an existing section of uncommented code
didn't make
sense to me. On the other hand, to maintain similar look'n'feel, I
haven't
been heavy-handed with comments in my new code. Of course, I will
make sure
it passes checkstyle before I actually cut the patches!
To put things in perspective, the tests only demonstrate that Mark's fix
works - and that wasn't even in question. However, I'd like to get my
change
committed soon, so that I can move forward with additional test cases.
What do you think? Should I publish and be damned, or would you like
me to
do more work to eliminate some of my compromises?
1. If you a set of changes (a, b, c) and you cannot separate them into
distinct patches, I would prefer to see (a), (a+b) and (a+b+c).
Seeing (a+b+c) is usually also good enough if you can explain the
changes such that a committer would be able to separate them.
Thanks for thinking about my worries, Konstantin. I appreciate you
spending time analysing what must appear to be a peripheral issue.
In fact, when I didn't receive a quick reaction, I started to think
along the same lines as you. At the moment, I am working on refactoring,
so that I can submit (a) entirely on its own, without breaking the
existing tests, but then being able to enable (b) as the next change.
Konstantin,
I would be very grateful if you could review the patch (actually the
second patch, which completely replaces the original) for this "step
(a)" enhancement:
https://issues.apache.org/bugzilla/show_bug.cgi?id=53960
The patch only changes HttpClient and is completely backward-compatible.
It passes checkstyle and a full run of the test suite on the latest
revision of the trunk (when, incidentally, some other tests were failing).
I hope it won't take much of your time and I would like to clear some
things from my pending list... this change would help me move on.
Thanks,
Brian
2. Why do you need sophisticated header parsing in SimpleHttpClient?
The server side for this client is always Tomcat, so its responses are
well known. Usually it is sufficient to check them for exact expected
values.
Yes, that is the philosophy of the current test suite. It checks the
expected behaviour of the tomcat example jsps, but in a manner that
makes the tests appear brittle to an outsider such as me.
I suppose I like to write tests that are a) extensible to explore edge
cases (I think there may be some not yet covered), and b) are robust
enough to handle external cosmetic changes to the unimportant aspects of
the test environment. e.g. if someone were to slightly alter the page
title of /examples/jsp/security/protected/index.jsp or login.jsp, many
of the FormAuthenticator tests would break... I don't feel comfortable
ignoring that possibility.
3. SimpleHttpClient is used when we need exact control over
transmitted data. In simpler cases we just use an URLConnection. See
TomcatBaseTest methods
getUrl(), postUrl(), methodUrl().
Yes, but the existing test suite is closely crafted around HttpClient
and has been in place (at least) since tomcat6. I assume the original
author wanted "exact control over transmitted data", but I haven't
worked my way through the other the test cases and so I don't know for
sure.
I saw no reason to think about throwing out the existing methodology and
prefer to try making it more flexible and robust (within reason).
Once again, thanks for your valuable opinions. I hope to submit the
first patches later this week. This discussion has provided useful
background which should make the code review much simpler for you.
Brian
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
