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

Show message in HTML report when source file can't be found #801

Merged

Conversation

Godin
Copy link
Member

@Godin Godin commented Dec 19, 2018

One of common problems already described in our FAQ and still frequently faced by users - incorrect location of source files, which leads to absence of pages with sources in HTML report:

So I propose to add following message to a page with class methods

Source file `org/example/Example.java` was not found during generation of report.

@Godin Godin requested a review from marchof December 19, 2018 13:44
@marchof
Copy link
Member

marchof commented Dec 19, 2018

@Godin Showing this hint when the source file cannot be found at the specified location is a good idea! But what if users do not provide source file at all (e.g. because they don't have it). In this case the same message would still be included in the report.

@Godin
Copy link
Member Author

Godin commented Dec 19, 2018

But what if users do not provide source file at all (e.g. because they don't have it). In this case the same message would still be included in the report.

@marchof I don't think that this little hint will bother such users.

@marchof
Copy link
Member

marchof commented Dec 21, 2018

@Godin In this case there are more common problems where we can add a hint to report:

  • Only class with different class id found
  • Class files without debug information

I would prefer to put the checks and message construction to a seperate, testable class DiagnosticsInfo.

@Godin
Copy link
Member Author

Godin commented Dec 21, 2018

In this case there are more common problems where we can add a hint to report:

  • Only class with different class id found
  • Class files without debug information

@marchof baby steps, I was planning to add these via follow-up PRs 😉

I would prefer to put the checks and message construction to a seperate, testable class DiagnosticsInfo.

Did I get it right - you just wanna pull entire if into separate class? i.e. without removal of dependency on HTMLElement body and ILinkable sourcePage ?

@marchof
Copy link
Member

marchof commented Dec 21, 2018

@Godin I haven't thought about the design in detail. My idea is to encapsulate the checks and the creation of warnings (as html elements) in a separate class. So for example the hint for missing line information can be shown on different page types.

@Godin
Copy link
Member Author

Godin commented Dec 21, 2018

@marchof I agree with idea 👍 Other than simple if extraction, design looks foggy to me for the time being because this message targets only single page. This message alone IMO is already quite valuable, so I'd like to propose merge as is and I'll work on addition of other messages and thus on the re-design in a next PR. Missing unit tests were added.

@Godin
Copy link
Member Author

Godin commented Dec 21, 2018

BTW should be noted that there are actually three cases of

Class files without debug information

  • javac -g:source produces only SourceFile attribute - line coverage can not be computed, however can link with source file without highlighting 😆
  • javac -g:lines produces only LineNumberTable attribute - line coverage can be computed, but can not link with source file
  • javac -g:none - no debug information at all

@marchof marchof merged commit ccad8eb into master Dec 23, 2018
@marchof marchof deleted the show_message_in_html_report_when_source_file_can_not_be_found branch December 23, 2018 19:51
@Godin Godin added this to the 0.8.3 milestone Dec 24, 2018
@jacoco jacoco locked as resolved and limited conversation to collaborators May 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants