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