RFR: Filing bug, ProblemListing, Backing out [v2]

[Phil Race]

On Fri, 3 Jul 2020 00:27:23 GMT, Jesper Wilhelmsson <jwilhelm at openjdk.org> wrote:

>> Filing bug, ProblemListing, Backing out
>
> Jesper Wilhelmsson has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fixed comments from review

Changes requested by prr (no project role).

src/next.md line 22:

> 21: When a new failure is found in the JDK a bug should be filed to describe and track the issue. Depending on your
> role in the OpenJDK you can either use the [Bug Report Tool](https://bugreport.java.com/) or, if you are Author in an
> OpenJDK Project, report the bug directly in the [JDK Bug System](https://bugs.openjdk.java.net/). Try to make the bug
> report as complete as possible to make it easier to triage and investigate the bug. It's not the reporter's
> responsibility to set a correct priority, but a good guess is always better than a default value. To help with setting
> the right priority consider things like how the bug impacts the product and our testing, how likely is it that the bug
> triggers, and how difficult is it to work around the bug if it's not fixed soon. 22:  23: A few things to keep in mind
> when filing a new bug:

and whether it is a recent regression, since that may break existing applications. regressions are often higher
priority than long standing bugs.

src/next.md line 42:

> 41: Sometimes tests break. It could be due to bugs in the test itself, or due to changed functionality in the code that
> the test is testing. While working on a fix, it can be useful to stop the test from being executed in everyone else's
> testing to reduce noise, especially if the test is expected to fail for more than a day. There are two ways to stop a
> test from being run in standard test runs: ProblemListing and using the `@ignore` keyword. Removing tests isn't the
> standard way to remove a failure. A failing test is expected to be a regression and should ideally be handled promptly
> with high urgency. 42:  43: I'll say it right away so that it's not forgotten at the end: Remember to remove the JBS id
> from the ProblemList or the test when the bug is closed. This is especially easy to forget if a bug is closed as a
> duplicate or 'Will Not Fix'.

this isn't the whole picture A test may fail because it is now being tested in  new environment, (eg new platform or
platform release) not just because of the afore stated reasons. This would not be a regression but might still be
important.

src/next.md line 48:

> 47: ProblemListing is used for a short term exclusion while a test is being fixed, and for the exclusion of
> intermittently failing tests that cause too much noise, but can still be useful to run on an ad-hoc basis.
> ProblemListing is done in the file `ProblemList.txt`. There are actually several ProblemList files to choose from.
> Their location and name hint about what area or feature each file belongs to. Each file has sections for different
> components. All ProblemList files complement each other to build the total set of tests to exclude in JTReg runs. 48:
> 49: ~~~

the emphasis that the exclusion is short lived is sadly unduly optimistic. Problem listing can be used for long term
exclusions too. It is still better than deleting a valid test.

src/next.md line 73:

> 72: In this example, `MyTest.java` is ProblemListed on windows, tracked by bug `JDK-4711`.
> 73:
> 74: Currently there's [no support for multiple lines for the same
> test](https://bugs.openjdk.java.net/browse/CODETOOLS-7902481). For this reason it's important to always make sure
> there's no existing entry for the test before adding a new one, as multiple entries might lead to unexpected results,
> e.g.

Not sure where in this review to insert this but there is a sensible policy that you should make sure the bug report is
open. If you are submitting it that is easy to ensure but there have been cases where bugs are closed (without
necessarily being fixed) and the issue remains PL'd. A legitimate case where this occurs if a bug is fixed in one line
of development but the fix is not (yet:?) backported and I'm not sure how the "sensible policy" accounts for that.

src/next.md line 147:

> 146:
> 147: If a change cause a regression that can't be fixed within reasonable time the best way to handle the regression
> can be to back out the change. Backing out means that the inverse (anti-delta) of the change is pushed to effectively
> undo the change in the repository. There are two parts to this task, how to do the bookkeeping in JBS, and how to do
> the actual backout in mercurial. 148:

cause -> causes

src/next.md line 149:

> 148:
> 149: The backout is a regular change and will have to go through the standard code review process, but is considered a
> trivial change and thus it requires only one Reviewer and will avoid the 24h code review window, even for areas where
> stricter rules usually applies. The rationale is that a backout is usually urgent in nature and the change itself is
> automatically created by hg, and reviewed by the person who is performing the backout, so even though only one
> additional Reviewer is required the change will in practice get two reviews. 150:

where is the 24h code review window discussed ?
It might be a sensible policy for non-urgent fixes but I've never seen it documented/codified and we've never had it as
a rule in the client area.

src/next.md line 154:

> 153: #. Close the original JBS issue **(O)**.
> 154:    * "Verify" the issue and choose "Fix Failed".
> 155: #. If the intention is to fix the change and submit it again, create a redo-issue **(R)** to track that the work
> still needs to be done.

I have strong objections to fix failed ever being used and oppose it being recommended here. Unless the fixer and their
reviewers completely failed at their job what you usually have is some other problem caused by the fix and the fix
actually succeeded.

-------------

PR: https://git.openjdk.java.net/guide/pull/21