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
<[email protected] <mailto:[email protected]>> 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
<[email protected] <mailto:[email protected]>> 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
<[email protected]
<mailto:[email protected]>> 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/
<http://cr.openjdk.java.net/%7Ejcbeyler/8219691/webrev.00/>
Bug: https://bugs.openjdk.java.net/browse/JDK-8219691
Thanks,
Derek.
