Modify

Opened 4 years ago

Closed 4 years ago

#12875 closed enhancement (fixed)

[Patch] Add data to bug report

Reported by: michael2402 Owned by: team
Priority: normal Milestone: 16.05
Component: Core bugreport Version:
Keywords: gsoc-core Cc: Don-vip, bastiK, stoecker

Description

See #12805

Adds a new error reporting class that allows you to add more information to a crash that happened.

I plan to extend the class BugReport to create the report text (status report, ...) in the next days.

I already added the catch code to MapView. Whenever an error happens during drawing the map, information about the layers that are displayed is printed. Those catch blocks don't cost any performance, so we can add as many as we want.

The report then looks like this:

=== REPORTED CRASH DATA ===
MapView#paintLayer:
 - layer: org.openstreetmap.josm.gui.layer.OsmDataLayer@6d4a427a
 - bounds: Bounds[49.4938256,8.0819993,49.4976837,8.0879016]

=== STACK TRACE ===
Thread: AWT-EventQueue-0 (16) of main
java.lang.RuntimeException: test
	at org.openstreetmap.josm.gui.layer.OsmDataLayer.paint(OsmDataLayer.java:408)
	at org.openstreetmap.josm.gui.MapView.paintLayer(MapView.java:740)
	at org.openstreetmap.josm.gui.MapView.paint(MapView.java:811)
	at javax.swing.JComponent.paintChildren(JComponent.java:879)
...

Attachments (2)

patch-bug-report-base.patch (5.8 KB) - added by michael2402 4 years ago.
patch-bug-report-base-tests.patch (14.4 KB) - added by michael2402 4 years ago.
Test cases

Download all attachments as: .zip

Change History (10)

comment:1 Changed 4 years ago by stoecker

Why not shorten the lines instead of adding an exception?

comment:2 Changed 4 years ago by Don-vip

It's because I think we need line numbers in the stacktrace. That's not the way to go indeed. Instead of adding an exception, it's better to add:

                // CHECKSTYLE.OFF: LineLength
                // @formatter:off
                // your long line
                // @formatter:on
                // CHECKSTYLE.ON: LineLength

this prevents Checkstyle to complain, and Eclipse formatter to mess with your code.

comment:3 Changed 4 years ago by michael2402

Yes, I can change that. I'll update the patches.

The problem is that two stack traces should be considered the same if all frames are the same (same file, same line number, ...). We could improve this some day (only consider the last X frames, ...). I intend to use this to give the user the ability to only suppress this one type of error.

comment:4 Changed 4 years ago by michael2402

I updated the patch. @formatter:on is disabled for the current formatter proflie, see #12877

comment:5 Changed 4 years ago by Don-vip

Thanks! Some remarks:

  • BugReport:
    • checkstyle: Utility classes should not have a public constructor
  • ReportedException:
    • findbugs: caughtOnThread should be transient
    • sonar: methodWarningFrom should be final
    • sonar: "thread.getKey() == caughtOnThread" should use equals instead
    • sonar: niceThreadName should be static
    • sonar: hasSameStackTrace should be static
    • sonar: "catch (Throwable t)" should be replaced by "catch (RuntimeException e)"
    • sonar: makeCollectionNice should be static
    • sonar: makeCollectionNice should use a StringBuilder instead of string concatenation
    • sonar: SectionEntry.print should be documented
    • sonar: Section.put and Section.printSection should be documented

You should configure Eclipse to use the Checkstyle, Findbugs and SonarLint plugins :)

comment:6 Changed 4 years ago by michael2402

Thanks for the hints. I installed the plugins and tested them.

I still have the following warnings:

  • BugReport constructor - OK I'll fix this next week when working on that class.
  • ReportedException methodWarningFrom / TODO - next week
  • ReportedExceptionTest: no assert in test case - I have not found an easy way to convince sonar that I simply want the test to not throw any exception. And in one case it did not get the assertEquals inside the loop.

Changed 4 years ago by michael2402

Attachment: patch-bug-report-base.patch added

Changed 4 years ago by michael2402

Test cases

comment:7 in reply to:  6 Changed 4 years ago by Don-vip

Replying to michael2402:

I still have the following warnings:

  • ReportedExceptionTest: no assert in test case - I have not found an easy way to convince sonar that I simply want the test to not throw any exception. And in one case it did not get the assertEquals inside the loop.

Me neither, that's OK. I don't know if I'll keep this rule enabled. Sometimes it's useful...

comment:8 Changed 4 years ago by Don-vip

Resolution: fixed
Status: newclosed

In 10285/josm:

fix #12875 - Add data to bug report (patch by michael2402)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.