On 29/03/17 07:06, Katya Todorova wrote: > On Tue, Mar 28, 2017 at 5:45 PM, Mark Thomas <ma...@apache.org> wrote: > >> On 28/03/17 15:23, Katya Todorova wrote: >>> Hi, >>>> r1787662 adds Host header validation along with a fair number of unit >> tests. >>>> It includes a performance test which indicates - on my machine at least >>>> - that the performance impact is in the noise. I'd like to see better >>>> performance for full IPv6 addresses but the current code looks to be >>>> acceptable. >>>> The validation is not yet integrated into the request processing. My >>>> primary reason for not integrating it is that it will trigger a 400 >>>> response if the header is invalid and I don't want to incorrectly reject >>>> valid headers. Therefore I have a request. Please try and break these >>>> new parsers. >>> >>> >>> I’ve looked at the new http host parsers and tried some test data. >>> Most of the test cases have already been covered but still several >>> issues popped up: >> >> Thanks for the additional test cases. This is exactly the sort of >> feedback I was looking for. >> >> Would you like to get more involved in Tomcat development? If so, >> turning these into a patch for the unit tests could be good place to >> start. You'll need to mark the tests with @Ignore for now until the >> underlying bugs are fixed. For bonus points, fix the bugs in the parser >> so the tests pass. >> > > I'll prepare a patch.
Excellent. > Would you prefer to keep the test cases more compact > or more extensive? > E.g. for testing IPv6 with less components than expected we may have one > test case with only 3 components (for example) or we may have many test > cases - respectively with one, two, ..., seven components? The latter > approach would make the test class a lot bigger but might help to catch > some corner cases. > > Let me know what you think. I recommend using the code coverage reports as a guide. https://ci.apache.org/projects/tomcat/tomcat9/coverage/ and add test cases if they increase code coverage. Hmm. It looks like there is some low hanging fruit in the parsing code to improve coverage. You can run the tests with code coverage locally too. Other than that, the usual advice applies. Test limits, either side of limits, etc Generally, I try and reduce copy and paste as much as possible. @RunWith has been really good for that. There is probably scope to use it more throughout the Tomcat tests. Kind regards, Mark --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
