RFR (XS) 8219691: method summary table head should be enclosed in <thead>

[Derek Thomson]

Thanks Jonathan,

I think these test are fine now I've worked with them a bit - I just didn't
want to build all this just to test my one case, basically. Anyway, a bunch
of tests do fail for this change and I'm working through them.

Another question: are all these tests expected to pass right now? It seems
like some fail in ways that have nothing to do with my change. If not, I
can produce another webrev for a fix for them next.

-- Derek.


On Thu, Mar 7, 2019 at 11:25 AM Jonathan Gibbons <
jonathan.gibbons at oracle.com> wrote:

> Derek,
>
> You may have already found the answer, but to be clear ... if I were
> writing the test for this, I would call "javadoc" with some suitable input
> file, and then call "checkOutput" to verify that a fragment of golden text
> is present in the generated file, where the fragment is just the beginning
> of the table, perhaps starting at `<table>` and ending at the first
> `<tr>`.   You could also do (instead) a shorter one going from the end of
> the caption to the beginning of the row, ensuring there is a `thead` in
> between.
>
> Some more notes:
>
>    - You may have come across methods like "checkLinks", "checkHeaders",
>    "checkAccessibility".  These are recent new methods that are typically
>    applied to the output of all runs of javadoc, because these are more robust
>    checks than hand-crafting specific tests.  They are worthwhile because they
>    cover output that is generated in many different parts of javadoc.   In
>    contrast, your fix is a fairly localized fix, and testing it once or twice
>    will likely be enough.
>
>    - The tests are somewhat brittle, but we have put a significant amount
>    of effort into making them less brittle over the years. (If you want
>    nightmares, go look at the tests as they were in the early days of OpenJDK!)
>
>    - Browser testing is all well and good, and we do some of that too,
>    but it is temporal and time-consuming.  It -is- important that we make sure
>    that code doesn't change when doing other work, and using diff would be a
>    "poor" (that's a British understatement) way to go because any change to
>    the generated files would break the test.
>
>    - Older tests use literal example code, but the signal-to-noise ratio
>    for these files is often through the roof because of the required legal
>    header. Newer tests tend to generate small examples on the fly, using
>    library classes like ToolBox, ModuleBuilder and ClassBuilder. Unless you
>    *really* need big example code, I would recommend using one of these
>    generator classes instead of using literal example code.
>
> -- Jon
>
>
> On 3/7/19 10:41 AM, Derek Thomson wrote:
>
> I take it back, I just found the tests that *use* the framework in another
> part of the tree. This is obviously what you meant!
>
> On Thu, Mar 7, 2019 at 10:32 AM Derek Thomson <dthomson at google.com> wrote:
>
>> Thanks for the feedback. I just found the JavadocTester code, thanks for
>> the pointer.
>>
>> The problem I'm having is that the tests don't currently check to this
>> level of detail, or at least I can't find where they do. The closest I can
>> find is some checks of the links. So, firstly, no test fails. To change
>> that, I'd have to write a specific test to check these specific tags are in
>> place. It's going to be a bit weird to have a test just for this thead
>> element and nothing else at the same level of detail. Also, I'm a big
>> proponent of testing and even test driven coding, and I really find that
>> checks of the lowest level HTML and CSS are really just tests that the code
>> didn't change (i.e. you might as well just use 'diff'), and are almost
>> entirely redundant, just mirror the code too closely, and are incredibly
>> brittle e.g. breaking due to whitespace changes. I usually test this sort
>> of thing with a browser driven rendering test that demonstrates that the
>> end result actually looks and behaves as expected, regardless of the
>> underlying mechanisms used to achieve that.
>>
>> All that said, I can definitely add a "CheckHeaders" test of some kind,
>> it just seems like an oddity right now. Unless you're trying to get
>> coverage from this time going forward by forcing everyone to write a new
>> test until better coverage is achieved, I can go along with that. I still
>> have my brittleness concerns, but a browser rendering test framework is a
>> major infrastructure investment too.
>>
>> I'll start tinkering with a test until you can follow up here anyway.
>>
>> Thanks again,
>> Derek.
>>
>> On Wed, Mar 6, 2019 at 5:32 PM Jonathan Gibbons <
>> jonathan.gibbons at oracle.com> wrote:
>>
>>> Hi Derek,
>>>
>>> Thanks for taking a look at this.
>>>
>>> The code itself looks reasonable, but I see you didn't provide anything
>>> in the way of a test.  There's two ways to look at that ..  either your
>>> change will break some existing tests, which will need to be fixed up, or
>>> it will not break any existing tests, indicating there is a lack of test
>>> coverage in this area, and so a test would be worth while. Either way, we
>>> should make sure there is test coverage for this change.
>>>
>>> Writing javadoc tests is pretty easy these days, using the
>>> "JavadocTester" framework.  Typically these days, for a case like this, I
>>> would expect to see a test generate a class with a few methods, perhaps
>>> using Toolbox.writeJavaFiles, then run it through javadoc, using
>>> JavadocTester.javadoc, and then call checkOutput to verify that the
>>> expected output has been generated.
>>>
>>> -- Jon
>>>
>>>
>>>
>>> On 03/06/2019 03:37 PM, Derek Thomson wrote:
>>>
>>> Hi all,
>>>
>>> I saw this bug and thought I could take a stab at it. Could someone
>>> review my change please?
>>>
>>> Webrev: http://cr.openjdk.java.net/~jcbeyler/8219691/webrev.00/
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8219691
>>>
>>> Thanks,
>>> Derek.
>>>
>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/javadoc-dev/attachments/20190307/c91a9a63/attachment.html>