Skip to content

Conversation

JesperIRL
Copy link
Member

@JesperIRL JesperIRL commented Jul 2, 2020


Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Reviewers

Download

$ git fetch https://git.openjdk.java.net/guide pull/21/head:pull/21
$ git checkout pull/21

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 2, 2020

👋 Welcome back jwilhelm! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label Jul 2, 2020
@mlbridge
Copy link

mlbridge bot commented Jul 2, 2020

Copy link
Member

@iignatev iignatev left a comment

Choose a reason for hiding this comment

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

Hi Jesper,

modulo a few comments, ProblemListing part looks good. thank you again for writing this.

@JesperIRL
Copy link
Member Author

Thank you Igor! All comments fixed and I also added the example for ProblemListing single test cases that you sent.

Copy link
Member

@iignatev iignatev left a comment

Choose a reason for hiding this comment

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

still LGTM

@AlanBateman
Copy link
Collaborator

Filing a Bug - are you going to say anything about checking if the issue exists in the main line, esp. with reproducible issues? Also what about advice on attaching a reproducer if possible?

"ProblemListing or @ignore-ing a Test". The ProblemList.txt files are jtreg exclude list. Can this section be renamed to "How to exclude tests"? I don't think we've used "@ignore" for long time and might it simpler to just not mention it.

@iignatev
Copy link
Member

Hi @AlanBateman,

I don't think we've used "@ignore" for long time and might it simpler to just not mention it.

@ignore is still used, albeit not very often, in hotspot tests for cases similar to the ones described here; given we are seldomly running ProblemList-ed tests, it would be really unfortunate to have include tests which remove /etc/shadow in such runs.

@JesperIRL
Copy link
Member Author

Filing a Bug - are you going to say anything about checking if the issue exists in the main line, esp. with reproducible issues? Also what about advice on attaching a reproducer if possible?

Good suggestion! I have added both.

"ProblemListing or @ignore-ing a Test". The ProblemList.txt files are jtreg exclude list. Can this section be renamed to "How to exclude tests"? I don't think we've used "@ignore" for long time and might it simpler to just not mention it.

I agree with changing the header of this section, and I do like "How to"-sections, but currently the guide doesn't have this style so I suggest simply "Excluding a test".

As for the usage of @ignore; the keyword is used in ~100 tests, about a third in each of hotspot, jdk, and langtools, so it's clearly used. I've added a few words to say that this is a less common use case, but I think the description as such should remain.

@iignatev
Copy link
Member

As for the usage of @ignore; the keyword is used in ~100 tests, about a third in each of hotspot, jdk, and langtools, so it's clearly used. I've added a few words to say that this is a less common use case, but I think the description as such should remain.

for the record, the discussion has continued on the mailing list (but due to some reasons hasn't been bridged to this PR), 8249618 has been filed to normalize/clean up usage of @ignore vs ProblemList.

@mlbridge
Copy link

mlbridge bot commented Aug 27, 2020

Mailing list message from Jesper Wilhelmsson on guide-dev:

On 26 Aug 2020, at 20:21, Kevin Rushforth <kcr at openjdk.java.net> wrote:
src/next.md line 28:

27: * Set affects version to the JDK version(s) where the failure was seen.
28: * If the failure is found in an update train of the JDK (e.g. 11.0.x), please make an effort to see if the bug is
also present in [mainline](https://hg.openjdk.java.net/jdk/jdk/). 29: * In the description, always include (if
possible):

This isn't really a comment on your proposed change. I presume there will be a follow-on task to update the docs after
next week when the jdk repo transitions from Mercurial to Git?

Yes, that will be a separate change as it will touch a lot more than just this part of the guide.
/Jesper

@AlanBateman
Copy link
Collaborator

One other comment on this is that "Filing a Bug" asks for labels to be added. Is there anywhere to link to that describes these labels? If not, then maybe this section could at least include a sentence about the "regression" label as it's a really useful indication that the reporter thinks there is a regression. If possible, the reporter should try to narrow down which release or build the regression first appeared as this can save a lot of time when looking at a bug report.

@JesperIRL
Copy link
Member Author

One other comment on this is that "Filing a Bug" asks for labels to be added. Is there anywhere to link to that describes these labels?

There is the JBS label dictionary that I intend to move into the guide. It's probably wise to do that sooner rather than later. Then each label can link to the description of that label. (https://wiki.openjdk.java.net/display/HotSpot/JBS+Label+Dictionary)

If possible, the reporter should try to narrow down which release or build the regression first appeared as this can save a lot of time when looking at a bug report.

Yes, that's a very good point. I've added a sentence about that.

Copy link
Member

@irisclark irisclark left a comment

Choose a reason for hiding this comment

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

Thanks for adding this information to the Guide and patiently working will reviewers to address their concerns.

@openjdk
Copy link

openjdk bot commented Sep 29, 2020

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

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

Filing bug, ProblemListing, Backing out

Reviewed-by: iignatyev, iris

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 5 new commits pushed to the master branch:

  • 061a3f8: Adding mapping from source to mail list
  • 62ad61b: Added pandoc version recommendation
  • c44972d: Removed blockquote formatting for code blocks.
  • ba1a32b: Use https for all links
  • 7704733: Css updates

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 label Sep 29, 2020
@JesperIRL
Copy link
Member Author

/integrate

@openjdk openjdk bot closed this Oct 1, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 1, 2020
@openjdk
Copy link

openjdk bot commented Oct 1, 2020

@JesperIRL Since your change was applied there have been 5 commits pushed to the master branch:

  • 061a3f8: Adding mapping from source to mail list
  • 62ad61b: Added pandoc version recommendation
  • c44972d: Removed blockquote formatting for code blocks.
  • ba1a32b: Use https for all links
  • 7704733: Css updates

Your commit was automatically rebased without conflicts.

Pushed as commit b552987.

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

@JesperIRL JesperIRL deleted the next branch October 1, 2020 04:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants