RFR: 8176453: Javadoc search: there are issues with generics in parameters

Wed Aug 29 14:40:43 UTC 2018

Sorry, I didn’t think it was necessary. Here it is:

http://cr.openjdk.java.net/~hannesw/8176453/webrev.03/

Hannes

> Am 29.08.2018 um 16:27 schrieb Jonathan Gibbons <jonathan.gibbons at oracle.com>:
> 
> Are you going to post a reduced webrev, without the Selenium files?
> 
> -- Jon
> 
> 
> On 8/29/18 2:55 AM, Hannes Wallnöfer wrote:
>> Thanks for the feedback, Jon.
>> 
>> I uploaded the generated docs to:
>> 
>> http://cr.openjdk.java.net/~hannesw/8176453/api/
>> 
>> It’s still uploading as I write this, but the search on the index page is working. Try anything with '<' or '>' to test the fix, such as „map<str“.
>> 
>> The suggestions for selenium tests and for a search spec all sound very reasonable and will keep me busy for a while.
>> 
>> Hannes
>> 
>>> Am 29.08.2018 um 00:12 schrieb Jonathan Gibbons <jonathan.gibbons at oracle.com>:
>>> 
>>> Hannes,
>>> 
>>> I suggest that we split the bug fix from the Selenium work.
>>> 
>>> That being said, if you do split it, I think it would be good to see a copy of the API docs built with the patch, and to have a written description of how to verify the bug (on standard docs) and the fix (on the modified docs).  You can build the docs with "make docs-jdk-api" and then rsync them into your directory on cr.ojn.
>>> 
>>> Separately, we should look at what it means to provide a test infrastructure for Selenium tests. I'm sure that we need more than a couple of test classes. I think we should start by writing a spec for Javadoc Search, so that we can design test cases based on that spec, and then build an extensible  test infrastructure that is capable of running those test cases, and any more that we may come up with as needed.
>>> 
>>> In terms of the Selenium tests you wrote,
>>> 
>>> * Consider moving the instructions to a README (or README.md) file
>>> 
>>> * Consider moving properties that might change (like the selenium driver and selenium driver URL to a .properties file so that "normal users" (i.e. anyone other that you) do not need to change the build.xml file
>>> 
>>> * Ant allows OS-specific operations, so it would be nice to try and download drivers for the browsers for the current platform, if it is one of Windows, Linux or macOS.  If you can't do it automatically, perhaps because you need special privileges, then you need really good detailed specific instructions, on what files to download and what commands to execute.
>>> 
>>> * Provide details of how to handle proxies, for folk behind a firewall.
>>> 
>>> * You currently have tests for a couple of bugs in the one file, JavaDocSearchTest.java.   I note the runTests() method (82-86) and the test methods in lines 89-104. Although this is OK as an initial proof of concept, it is not going to scale well, if we want more complete tests for javadoc search functionality.  I would expect that (eventually?) we will want to have tests spread across multiple files, perhaps using infrastructure like TestNG or JUnit or something equivalent, to discover and run test cases.
>>> 
>>> * Stylistically, you have lots of hard-coded references to System.err.  It would be better to use a member variable, maybe called "log" that is initialized up front to a stream like System.err.
>>> 
>>> * The hard-coded limit of 5 seconds in searchResults is a bit scary.  Should that be either configurable, or at least called out as a named constant.
>>> 
>>> * It looks like the test requires that the system property docs.dir needs to be set up. This should be documented, so that the test can be run standalone, such as outside of Ant (e.g. from an IDE). It would be good for the test to verify that the property has been set, and report an error if not.
>>> 
>>> -- Jon
>>> 
>>> On 08/14/2018 07:36 AM, Hannes Wallnöfer wrote:
>>>> I’ve uploaded a new webrev:
>>>> 
>>>> http://cr.openjdk.java.net/~hannesw/8176453/webrev.02/
>>>> 
>>>> This allows to set the build/test artefact directory using the working.dir system property:
>>>> 
>>>> ant -Dworking.dir=…
>>>> 
>>>> By default the current working directory is used as I don’t think there’s a safe default location that works cross platform.
>>>> 
>>>> It is also possible to set the lib.dir property to another directory containing selenium-server-standalone-3.13.0.jar so it will not attempt the download. However the download is still attempted if the file is not present.
>>>> 
>>>> The only problem with adding (lots of) classes/packages/modules is repository bloat. As an alternative, it would be quite easy to point the tests to other, existing classes belonging to the neighbouring tests.
>>>> 
>>>> Hannes
>>>> 
>>>> 
>>>>> Am 14.08.2018 um 01:43 schrieb Jonathan Gibbons <jonathan.gibbons at oracle.com>:
>>>>> 
>>>>> Hannes,
>>>>> 
>>>>> This looks like a good start.
>>>>> 
>>>>> It would be better if you did not always download/install Selenium.  It should be possible to run the tests with the Ant file configured to point to a local already-installed version of Selenium.  If nothing else, this helps avoid problems related to proxies.   Yes, I know that you set skipexisting="true" but you'll still download Selenium each time in a fresh repo or after "make clean" etc.
>>>>> 
>>>>> Without looking at it in great depth, it looks like the test might write files in the main test/ directory (or some subdirectory thereof). That is generally bad practice. Think of the src/ and test/ dirs in the repo as read-only. You should be able to configure where any files will be written.
>>>>> 
>>>>> How easy/practical will it be to update the test code (that you run through javadoc) to have many modules/packages/classes so that Javadoc Search has lots to work on?  i.e. lots of test cases within it.
>>>>> 
>>>>> -- Jon
>>>>> 
>>>>> 
>>>>> On 8/9/18 9:14 AM, Hannes Wallnöfer wrote:
>>>>>> Sundar, Jon: thanks for the reviews, and sorry for the late reply (I was on vacation last week).
>>>>>> 
>>>>>> As suggested I went ahead and wrote some Selenium tests for this and the previous search bug I fixed, along with a simple Ant build file to run them. I included this in the webrev in test/langtools/jdk/javadoc/doclet/seleniumTests - not sure if this a proper location. It includes its own test sources for generating API docs.
>>>>>> 
>>>>>> http://cr.openjdk.java.net/~hannesw/8176453/webrev.01/
>>>>>> 
>>>>>> Instructions for running the tests are included as comment in the main test class (JavaDocSearchTest.java). Unfortunately browser drivers still require manual configuration as these are highly platform specific.
>>>>>> 
>>>>>> Some additional comments:
>>>>>> 
>>>>>> - I’m pretty sure that ‚&‘ will not occur in search results as type parameters are not displayed and wildcards cannot have multiple bounds. We could still escape it to be sure, but I thought it was more important to keep the overhead to a minimum and so kept it at just ‚<‚ and ‚>'.
>>>>>> 
>>>>>> - The ant task to download the Selenium jar file uses the HTTP URL instead of secure HTTPS. This is because HTTPS fails for me, probably because I did not include closed sources.
>>>>>> 
>>>>>> Let me know if you think this is the way to go.
>>>>>> 
>>>>>> Hannes
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>>> Am 31.07.2018 um 00:39 schrieb Jonathan Gibbons <jonathan.gibbons at oracle.com>:
>>>>>>> 
>>>>>>> While parameterized types typically just contain < and >, the bounds may sometimes include &, so I tend to agree with Sundar that if you need to escape < > you probably need to escape & as well.
>>>>>>> 
>>>>>>> Even if we can't automate tests (yet?) I think it would be good for you to set up test cases for manual tests that allow us to check the behavior on the OS and browser of our choice.  Simply asking us to test this is not enough.
>>>>>>> 
>>>>>>> If you can find test cases that leverage the latest JDK API, then you could point at that. Otherwise you could construct test cases and post them near the webrev on your account as cr.ojn.  Sometimes I will just build the latest JDK API and post that on cr.ojn.
>>>>>>> 
>>>>>>> -- Jon
>>>>>>> 
>>>>>>> 
>>>>>>> On 07/30/2018 07:19 AM, Sundararajan Athijegannathan wrote:
>>>>>>>> Don't you also need ".replace(/&/g,'&')"?
>>>>>>>> 
>>>>>>>> PS. Isn't there a standard way to escape HTML in JS?
>>>>>>>> 
>>>>>>>> 
>>>>>>>> -Sundar
>>>>>>>> 
>>>>>>>> On 27/07/18, 7:51 PM, Hannes Wallnöfer wrote:
>>>>>>>>> Please review and test this patch to escape < and > characters in search results to HTML < and > entities.
>>>>>>>>> 
>>>>>>>>> Issue:
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8176453
>>>>>>>>> 
>>>>>>>>> Webrev:
>>>>>>>>> http://cr.openjdk.java.net/~hannesw/8176453/webrev.00/
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> This fixes rendering of search results for search terms containing generics such as „map<str“.
>>>>>>>>> 
>>>>>>>>> Thanks,
>>>>>>>>> Hannes
>>>>>>>>> 
>>>>>>>>> 
> 