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 <[email protected]>: > > 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 >>> <[email protected]>: >>> >>> 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 >>>>> <[email protected]>: >>>>> >>>>> 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 >>>>>>> >>>>>>> >
