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

jcheck: add "problemlists" check #518

Closed
wants to merge 10 commits into from
Closed

Conversation

iignatev
Copy link
Member

@iignatev iignatev commented Mar 19, 2020

Hi all,

This is more of a request for comments than an actual PR.

The idea behind the patch is to ensure that whenever someone fixes a bug, its ID is not used in any of the problem lists. As some of you might know this is not a hypothetical issue, people forget to clean problem lists quite often, which in some cases results in poorly tested patches. Although it's true that even if we ensure that all commits are correct, there still will be ways to have "fixed" bugs in problem lists, e.g. closing as WNF, duplicate, etc, it's nevertheless believed that the majority (or the most annoying) "violations" are caused by commits.

The proposed check parses all problem lists specified by dirs (bar-separated list of test suites) and pattern (regexp pattern for files withing the dirs) and verifies that none of the issues fixed by a patch is listed in any of problem lists. The original / python-time patch used a list glob-pattern, but unfortunately, Java SE doesn't have a good(fast) API to find all files matching a glob, and I didn't want to bring 3rd party dependency just for this use-case.

Thanks,
-- Igor


Progress

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

Reviewers

  • Erik Helin (ehelin - Reviewer) ⚠️ Review applies to 24310cd

Download

$ git fetch https://git.openjdk.java.net/skara pull/518/head:pull/518
$ git checkout pull/518

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 19, 2020

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

@edvbld
Copy link
Member

@edvbld edvbld commented Mar 19, 2020

Hi Igor,

thanks for contributing 🎉

We are in general very positive to bring in additional "jchecks" to upstream since they can easily be configured per repository. The different OpenJDK projects usually choose the checks they want enabled/disabled themselves.

I think the check you are proposing makes a lot of sense, I've run into this scenario myself in the past. I had a brief look at the code, will do a more in-depth review later, but at a first glance it looks really good 👍 One thing that stands out: could you please add one or more unit tests for your new check? You can use the unit tests for other checks as an inspiration, please see the tests in jcheck/src/test/java/org/openjdk/skara/jcheck.

Thanks,
Erik

@iignatev
Copy link
Member Author

@iignatev iignatev commented Mar 19, 2020

Hi Erik,

Thanks for your prompt response. One of the reasons I haven't written any tests for this check is that I 1st wanted to get opinions on whether such a check will be accepted to skara/jcheck as my previous experience with jcheck.py showed that it's near to impossible to extend it with any new checks. Another reason is that this check reads the content of files in a repo, which AFAICT none of the existing tests do. Anyhow, I'll work on developing tests.

Cheers,
-- Igor

@edvbld
Copy link
Member

@edvbld edvbld commented Mar 19, 2020

Ah ok, I get it. Skara's jcheck is very extensible, it is perfectly fine for a repository to start requiring more checks after a given commit by updating .jcheck/conf. It is also fine for a repository to only utilize a subset of the available checks (this is very common in practice).

Yes and no, the other checks do read at least .jcheck/conf 😄 What you need to be careful with is which revision you read the file contents from. You probably want to use ReadOnlyRepository::lines instead of reading a file directly from disk (since a user might jcheck an earlier commit than the one currently being checked out). It should be fairly easy writing a unit test showcasing this scenario 👍

Thanks,
Erik

@iignatev iignatev marked this pull request as ready for review Mar 19, 2020
@openjdk openjdk bot added the rfr label Mar 19, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 19, 2020

Webrevs

@iignatev
Copy link
Member Author

@iignatev iignatev commented Mar 20, 2020

Hi Erik,

thanks for the hint, I've updated the check to use ReadOnlyRepository to read files' content and list files in a directory. I've also added unit tests to cover different scenarios and configuration combinations.

there is one thing which I would like to discuss before we (hopefully) get it integrated -- the default value for dirs. as ProblemListsCheck will most probably be used only by "jdk-based" project, it might make sense to use values which reflects jdk layout (test/jdk|test/langtools|test/hotspot/jtreg|test/nashorn|test/jaxp) so most projects will be able to use the default value, on the other hand, I don't want to bake in the current directory layout, so I defaulted dirs to test which thought isn't good for any of the existing OpenJDK projects. so what do you think?

BTW, should I update copyright years in all the touched files?

Thanks,
-- Igor

@iignatev iignatev changed the title WIP/RFC: added ProblemLists check to jcheck add ProblemLists check to jcheck Mar 20, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Mar 23, 2020

@iignatev this pull request can no longer be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout master
git fetch https://git.openjdk.java.net/skara master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@edvbld edvbld changed the title add ProblemLists check to jcheck jcheck: add "problemlists" check Mar 23, 2020
@openjdk openjdk bot removed the merge-conflict label Mar 23, 2020
edvbld
edvbld approved these changes Mar 23, 2020
Copy link
Member

@edvbld edvbld left a comment

Looks good!

@openjdk
Copy link

@openjdk openjdk bot commented Mar 23, 2020

@iignatev This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

jcheck: add "problemlists" check

Reviewed-by: ehelin
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there has been 1 commit pushed to the master branch. Since there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 334899a73fd561b0bb9c334cf8c53f4cac9b6c62.

As you do not have Committer status in this project, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@edvbld) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Mar 23, 2020
@edvbld
Copy link
Member

@edvbld edvbld commented Mar 23, 2020

Great work on the patch and on the tests Igor, this is a really solid contribution! 🙌
As you probably noticed I took the liberty of editing the title of this pull request to align more with Skara's (totally undocumented) conventions, I hope you don't mind?

there is one thing which I would like to discuss before we (hopefully) get it integrated -- the default value for dirs. as ProblemListsCheck will most probably be used only by "jdk-based" project, it might make sense to use values which reflects jdk layout (test/jdk|test/langtools|test/hotspot/jtreg|test/nashorn|test/jaxp) so most projects will be able to use the default value, on the other hand, I don't want to bake in the current directory layout, so I defaulted dirs to test which thought isn't good for any of the existing OpenJDK projects. so what do you think?

I think defaulting to test is fine, I prefer that OpenJDK projects have to specify the directories containing ProblemList.txt files in their .jcheck/conf. That makes it clear which files that will actually be considered.

BTW, should I update copyright years in all the touched files?

Yes please 🙏

@iignatev
Copy link
Member Author

@iignatev iignatev commented Mar 23, 2020

Erik, thanks for reviewing this! I've updated copyrights in (hopefully) all the files. and yeah, I'm totally fine with the updated PR's title.

@iignatev
Copy link
Member Author

@iignatev iignatev commented Mar 23, 2020

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Mar 23, 2020

@iignatev
Your change (at version 69d89d3) is now ready to be sponsored by a Committer.

@openjdk openjdk bot added the sponsor label Mar 23, 2020
@edvbld
Copy link
Member

@edvbld edvbld commented Mar 23, 2020

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Mar 23, 2020

@edvbld @iignatev The following commits have been pushed to master since your change was applied:

  • 334899a: hg: add support for skara and focus on webrev, defpath and jcheck

Your commit was automatically rebased without conflicts.

Pushed as commit 0fec136.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 23, 2020

Mailing list message from Erik Helin on skara-dev:

Changeset: 0fec136
Author: Igor Ignatyev <iignatyev at openjdk.org>
Committer: Erik Helin <ehelin at openjdk.org>
Date: 2020-03-23 15:58:03 +0000
URL: https://git.openjdk.java.net/skara/commit/0fec136a

jcheck: add "problemlists" check

Reviewed-by: ehelin

! bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestCheckIssueVisitor.java
! cli/src/main/java/org/openjdk/skara/cli/JCheckCLIVisitor.java
! jcheck/src/main/java/org/openjdk/skara/jcheck/ChecksConfiguration.java
! jcheck/src/main/java/org/openjdk/skara/jcheck/IssueVisitor.java
! jcheck/src/main/java/org/openjdk/skara/jcheck/JCheck.java
+ jcheck/src/main/java/org/openjdk/skara/jcheck/ProblemListsCheck.java
+ jcheck/src/main/java/org/openjdk/skara/jcheck/ProblemListsConfiguration.java
+ jcheck/src/main/java/org/openjdk/skara/jcheck/ProblemListsIssue.java
! jcheck/src/test/java/org/openjdk/skara/jcheck/JCheckTests.java
+ jcheck/src/test/java/org/openjdk/skara/jcheck/ProblemListsCheckTests.java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants