Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8273544: Increase test coverage for snippets #6359

Closed
wants to merge 19 commits into from

Conversation

pavelrappo
Copy link
Member

@pavelrappo pavelrappo commented Nov 11, 2021

This change is mostly about tests. I say "mostly" because while increasing code coverage, which was the primary goal of this exercise, I uncovered a few non-critical bugs and fixed them in-place. The net effect of the change boils down to these code coverage statistics.

before

%method %block %branch %line
jdk.javadoc.internal.doclets.toolkit.taglets.SnippetTaglet
75%(12/16)
87%(109/124)
88%(99/112)
85%(140/164)

#classes %method %block %branch %line
jdk.javadoc.internal.doclets.toolkit.taglets.snippet
70%(80/114)
76%(310/407)
65%(178/273)
81%(413/508)

after

%method %block %branch %line
jdk.javadoc.internal.doclets.toolkit.taglets.SnippetTaglet
100%(17/17)
95%(120/126)
93%(103/110)
97%(163/168)

%method %block %branch %line
jdk.javadoc.internal.doclets.toolkit.taglets.snippet
83%(94/112)
85%(348/405)
73%(202/273)
91%(463/505)


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6359/head:pull/6359
$ git checkout pull/6359

Update a local copy of the PR:
$ git checkout pull/6359
$ git pull https://git.openjdk.java.net/jdk pull/6359/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6359

View PR using the GUI difftool:
$ git pr show -t 6359

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6359.diff

pavelrappo added 15 commits Nov 11, 2021
After JDK-8267636 has been integrated, jdk.javadoc is compiled using JDK 17. So we can finally use sealed classes where they were originally envisioned.
8266666 didn't test this option. This commit adds such a test with the "@bug 8266666" declaration. The test improves code coverage of jdk.javadoc.internal.doclets.toolkit.taglets.SnippetTaglet as follows:

            %method     %block      %branch    %line
            -----------+-----------+----------+------------
    before  100%(11/11) 88%(92/104) 84%(81/96) 88%(128/145)
    after   100%(11/11) 90%(94/104) 89%(86/96) 89%(130/145)

Note that the table might become outdated due to rebases.
 - subclasses JavadocTester to provide reusable check and utility methods that will be shared by multiple snippet test classes
 - fixes a stupid regex mistake: \s -> \\s (filed a bug https://youtrack.jetbrains.com/issue/IDEA-279378)
 - re-orders some imports
The new test improves code coverage of jdk.javadoc.internal.doclets.toolkit.taglets.snippet as follows:

            %method     %block      %branch    %line
            -----------+------------+------------+------------
    before  70%(80/114) 76%(310/407) 65%(178/273) 81%(413/508)
    after   82%(94/114) 84%(344/407) 73%(202/273) 90%(459/508)

Note that the table might become outdated due to rebases.
  - removes dead code
  - uses autogenerated getters instead of field access for records
Changes tests names to match the terminology change that happened long ago: redundant snippets -> hybrid snippets.
A test may exercise both markup and tag syntax. Generally speaking, a test may be a part of multiple hierarchies. To model that, tests should be organized using tags (i.e. labels). A name component can be such a tag.
This commit immediately removes some amount of repetition.
Ideally, these tests should be implemented using hybrid snippets, to show how inline and external parts differ in treatment of */ and \uxxxx.
Increases test coverage for the "region" tag attribute. Fixes the bug where the valueless "region" attribute caused a crash (NPE).
Unifies error handling in the taglet by extracting an internal method and wrapping that method in try-catch. Fixes a few bugs where the attribute unexpectedly lacks value.
Covers extra 5 lines of SnippetTaglet: [244, 248]
Covers extra 2 lines of MarkupParser: [101, 102]
@bridgekeeper
Copy link

bridgekeeper bot commented Nov 11, 2021

👋 Welcome back prappo! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Nov 11, 2021

@pavelrappo The following label will be automatically applied to this pull request:

  • javadoc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the javadoc javadoc-dev@openjdk.org label Nov 11, 2021
@pavelrappo pavelrappo marked this pull request as ready for review Nov 11, 2021
@openjdk openjdk bot added the rfr Pull request is ready for review label Nov 11, 2021
@mlbridge
Copy link

mlbridge bot commented Nov 11, 2021

Webrevs

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons left a comment

In general, great work. Most of my comments are about style, especially unindented text blocks.

The one aspect that I think deserves attention is the code for the @link feature and the use of in-memory file objects and file managers. While not wrong, it seems overkill and unnecessary, since I think you could embed the reference tag and the markup tag in the same file, without going all the way to the separate run of javadoc. Given that it's not wrong, and that there are dependent reviews blocked by this, I'll approve it if you want, but would like to understand if there are reasons to do it as you have.

@@ -165,24 +199,21 @@ public Content getInlineTagOutput(Element holder, DocTree tag, TagletWriter writ
if (fileObject == null && fileManager.hasLocation(Location.SNIPPET_PATH)) {
fileObject = fileManager.getFileForInput(Location.SNIPPET_PATH, "", v);
}
} catch (IOException | IllegalArgumentException e) {
} catch (IOException | IllegalArgumentException e) { // TODO: test this when JDK-8276892 is integrated
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: The documented conditions of IllegalArgumentException do not arise in this context, although it's not wrong to catch the exception.

Copy link
Member Author

@pavelrappo pavelrappo Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are much more knowledgeable in FileManager than I am. My thinking was that while it's probably not an issue with the default file manager, I couldn't guarantee that IllegalArgumentException is not thrown if this code runs with a custom file manager.

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just reading the documented conditions for this JavaFileManager method to throw IllegalArgumentException. That being said, one of the problems with ILA, and potentially other unchecked exception, is that they can occur from deep within the method that is called.

FYI, javac has a way of wrapping user-provided components so that it can better handle unexpected exceptions from user-provided code as compared to unexpected exceptions in javac itself. If you're interested, seejavac ClientCodeWrapper. And, to be clear, I'm not suggesting that for javadoc at this time!

Copy link
Member Author

@pavelrappo pavelrappo Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That being said, one of the problems with ILA, and potentially other unchecked exception, is that they can occur from deep within the method that is called.

Care to elaborate?

@@ -79,7 +79,7 @@ record Replacement(int start, int end, String value) { }
// there's no need to call matcher.appendTail(b)
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(minor) maybe add a brief reason to the comment

* immediately found, the search is not abandoned.
*/
@Test
public void testSearchPath(Path base) throws Exception {
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not wrong, but arguably somewhat superfluous. This is close to being a file manager test, not a snippet test.

Copy link
Member Author

@pavelrappo pavelrappo Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add a comment that this test is a part of an end-to-end demonstration of how --snippet-path should work. This is a black-box test that assumes nothing about the implementation of the feature.

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, but I still think the test is somewhat unnecessary. But now you have it, don't delete it.

"""
: error: snippet markup: missing attribute "region"
// @start
^""");
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(style) why not indent the text?

 - Hoist the tb (toolbox) field to SnippetTester
 - Use use default ctors instead of private no-arg ctors for test classes
 - Re-implement SnippetTester.checkOutputEither on top of JavadocTester.OutputChecker.checkAnyOf
 - Follow convention for jtreg comment
 - Improve method names with underscores
var strings = Stream.concat(Stream.of(first), Stream.of(other))
.toArray(String[]::new);
new OutputChecker(out).checkAnyOf(strings);
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Nov 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works, and is a clever solution for now. Maybe "one of these days" we remove the checkOutputEither method.

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons left a comment

While I disagree with some of the code here, the code is not wrong, and so I'll approve the review to unblock the dependent work.

I hope we can clean up some of the issues later on.

Setting those concerns aside, overall, this is great work. Well done for such a thorough job for improving test coverage.

@openjdk
Copy link

openjdk bot commented Nov 19, 2021

@pavelrappo This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8273544: Increase test coverage for snippets

Reviewed-by: jjg

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 117 new commits pushed to the master branch:

  • 2d4af22: 8277370: configure script cannot distinguish WSL version
  • a3406a1: 8277092: TestMetaspaceAllocationMT2.java#ndebug-default fails with "RuntimeException: Committed seems high: NNNN expected at most MMMM"
  • e47cc81: 8275386: Change nested classes in jdk.jlink to static nested classes
  • f609b8f: 8274946: Cleanup unnecessary calls to Throwable.initCause() in java.rmi
  • 36b59f3: 8274333: Redundant null comparison after Pattern.split
  • 6677554: 8274949: Use String.contains() instead of String.indexOf() in java.base
  • 09e8c8c: 8277342: vmTestbase/nsk/stress/strace/strace004.java fails with SIGSEGV in InstanceKlass::jni_id_for
  • 976c2bb: 8277212: GC accidentally cleans valid megamorphic vtable inline caches
  • 03f8c0f: 8275887: jarsigner prints invalid digest/signature algorithm warnings if keysize is weak/disabled
  • 936f7ff: 8276150: Quarantined jpackage apps are labeled as "damaged"
  • ... and 107 more: https://git.openjdk.java.net/jdk/compare/2ca4ff87b7c31d56542bbdcea70e828be33f4e97...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 19, 2021
@pavelrappo
Copy link
Member Author

pavelrappo commented Nov 19, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Nov 19, 2021

Going to push as commit 2ab43ec.
Since your change was applied there have been 117 commits pushed to the master branch:

  • 2d4af22: 8277370: configure script cannot distinguish WSL version
  • a3406a1: 8277092: TestMetaspaceAllocationMT2.java#ndebug-default fails with "RuntimeException: Committed seems high: NNNN expected at most MMMM"
  • e47cc81: 8275386: Change nested classes in jdk.jlink to static nested classes
  • f609b8f: 8274946: Cleanup unnecessary calls to Throwable.initCause() in java.rmi
  • 36b59f3: 8274333: Redundant null comparison after Pattern.split
  • 6677554: 8274949: Use String.contains() instead of String.indexOf() in java.base
  • 09e8c8c: 8277342: vmTestbase/nsk/stress/strace/strace004.java fails with SIGSEGV in InstanceKlass::jni_id_for
  • 976c2bb: 8277212: GC accidentally cleans valid megamorphic vtable inline caches
  • 03f8c0f: 8275887: jarsigner prints invalid digest/signature algorithm warnings if keysize is weak/disabled
  • 936f7ff: 8276150: Quarantined jpackage apps are labeled as "damaged"
  • ... and 107 more: https://git.openjdk.java.net/jdk/compare/2ca4ff87b7c31d56542bbdcea70e828be33f4e97...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Nov 19, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 19, 2021
@openjdk
Copy link

openjdk bot commented Nov 19, 2021

@pavelrappo Pushed as commit 2ab43ec.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated javadoc javadoc-dev@openjdk.org
3 participants