tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Brian Burch <br...@pingtoo.com>
Subject Re: FormAuthenticatorTest for cases without cookies - implementation issues
Date Sat, 06 Oct 2012 16:20:15 GMT
On 30/09/12 23:46, Brian Burch wrote:
> On 30/09/12 21:56, Konstantin Kolinko wrote:
>> 2012/9/26 Brian Burch <brian@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-unsubscribe@tomcat.apache.org
>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>
>


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message