-
-
Notifications
You must be signed in to change notification settings - Fork 793
ICU-12717 Create ErrorProne reports to find issues #3487
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
base: main
Are you sure you want to change the base?
Conversation
Generates two kinds of reports, but with the same data. Sample here: |
8dfa6a1
to
9ee7c5f
Compare
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work!
|
||
File: sorttable.js (only for ICU4J) | ||
|
||
The MIT Licence, for code from kryogenix.org |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI
should be ok, since we already have JSON also under the MIT license
strangely, they differ somewhat -- JSON has a "notice" clause
icu4j/tools/build/src/main/java/com/ibm/icu/dev/tool/errorprone/ErrProneReport.java
Outdated
Show resolved
Hide resolved
icu4j/tools/build/src/main/java/com/ibm/icu/dev/tool/errorprone/ErrProneUtils.java
Outdated
Show resolved
Hide resolved
icu4j/tools/build/src/main/java/com/ibm/icu/dev/tool/errorprone/ErrProneUtils.java
Outdated
Show resolved
Hide resolved
icu4j/tools/build/src/main/java/com/ibm/icu/dev/tool/errorprone/HtmlUtils.java
Outdated
Show resolved
Hide resolved
icu4j/tools/build/src/main/java/com/ibm/icu/dev/tool/errorprone/ParseMavenOutForErrorProne.java
Outdated
Show resolved
Hide resolved
icu4j/tools/build/src/main/java/com/ibm/icu/dev/tool/errorprone/ParseMavenOutForErrorProne.java
Outdated
Show resolved
Hide resolved
icu4j/tools/build/src/main/java/com/ibm/icu/dev/tool/errorprone/ParseMavenOutForErrorProne.java
Outdated
Show resolved
Hide resolved
icu4j/tools/build/src/main/java/com/ibm/icu/dev/tool/errorprone/ParseMavenOutForErrorProne.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Markus Scherer <[email protected]>
…e/ErrProneUtils.java Co-authored-by: Markus Scherer <[email protected]>
…e/ErrProneUtils.java Co-authored-by: Markus Scherer <[email protected]>
…e/HtmlUtils.java Co-authored-by: Markus Scherer <[email protected]>
…e/ParseMavenOutForErrorProne.java Co-authored-by: Markus Scherer <[email protected]>
…e/ParseMavenOutForErrorProne.java Co-authored-by: Markus Scherer <[email protected]>
* | ||
* <p>Very similar to `com.google.errorprone.BugCheckerInfo` | ||
*/ | ||
class ErrorEntry { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: rename ErrorEntry
to:
ErrorProneIssue
ErrorProneItem
ErrorProneBug
ErrorProneEntry
IssueInstance
DetectedIssue
IssueLogItem
Just the word "Error" without fully saying "ErrorProne" sounds misleading or not fully specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
icu4j/tools/build/src/main/java/com/ibm/icu/dev/tool/errorprone/ErrorProneUtils.java
Outdated
Show resolved
Hide resolved
import com.google.errorprone.BugCheckerInfo; | ||
import com.google.errorprone.scanner.BuiltInCheckerSuppliers; | ||
|
||
class ErrorProneReport { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Is it possible to refactor this to allow for another mode that products Markdown formatted output?
The reason is that Github doesn't have any easy or built in way to serve up HTML per workflow run. Anyone who creates reports & dashboards faces this problem . Even if you use Github Pages to display a dashboard, you really only want to generate that from the upstream repo's main branch. Which means that for PRs, for every commit that you push to a PR before you merge, you don't really have anywhere to put the reports generated per commit.
The good thing is that GHA Job Summaries does support the ability to put a report somewhere regardless of which branch it comes from... It just needs to be in the form of Markdown. And the "somewhere" that it supports as the destination for the report is the workflow instance's summary page.
The Markdown formatting is Github Flavored Markdown (GFM). It will end up looking similar to what you have in HTML format, but not quite as fancy or feature-ful. As a potential user, I can live with less fancy formatting if I can have the convenience of having some type of accessible usable report on every push.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Is it possible to refactor this to allow for another mode that products Markdown formatted output?
Yes. But we can't do JavaScript. So we can't sort the table(s)
The second style (errorprone2.html) would probably work even without sorting.
But the 1st style (errorprone1.html) would not.
I was not thinking about this for regular PRs, but for something like a nightly build.
You can only enable something like this for each PR after we get the number of issues down to virtually nothing.
Otherwise we need something a lot fancier.
We would need to keep a history (before / after PR) and the ability to filter older failures.
If not, we would have to report something like: before your change we had 976 errorprone issues, with your PR we have 980 issue. Good luck finding them :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
way to serve up HTML per workflow run
Thinking about it, that feels weird. There should be a way...
Anyway, that is something I wanted to talk to you about.
When to run it, and where to post the results.
…e/ErrorProneUtils.java Co-authored-by: Elango Cheran <[email protected]>
Checklist