Add snippet code in html report #1975

BraisGabin · 2019-10-03T15:42:34Z

I'm trying to solve #1779

For get this done I added kotlinx.html library, I hope it's ok.

This implemetation is really basic. It creates things like this:

Problems that I see and I want some feedback:

~~I'm not using any prettify library to give a bit of color to the code~~ out of scope
~~I don't know what to test exactly~~
~~I'm opening the file directly from the path... This have problems, for example for testing because the files doesn't exist. The if (file.exist()) is a workaround, we should look for a good solution~~
~~If the error is in more than one line I just highlight the first line.~~
For some rules the snippet has no sense at all so we are wasting scroll space. We should have something to decide if we want to show the snippet or not.

On the other hand I copy&pasted some html code from the android linter. I don't know how to point that out in the code.

schalkms · 2019-10-04T19:39:49Z

First all, thank you for picking this up and for formulating your ideas in detail! That's an awesome contribution.
I'll try to answer your questions in the following lines.

I'm not using any prettify library to give a bit of color to the code

That's fine for version 1. I think you meant syntax coloring, right. There's probably a library that does syntax coloring for Kotlin.

I don't know what to test exactly

I haven't reviewed it in detail. FlowContent.snippetCode() and FlowContent.writeErrorLine() should be internal util functions that are tested with unit tests.
Furthermore, (almost) every branch should be covered to maintain the code coverage.
There should also be 1 integration test that tests the HTML report module.

I'm opening the file directly from the path

The file writing process happens in the abstract base class called OutputReport. The derived class HtmlOutputReport should not implement this for testability reasons.

If the error is in more than one line I just highlight the first line.

This should be tested thoroughly. There are many cases that could be covered by unit tests.

Reported at the beginning of the file.
Reported at the end of the file.
...

For some rules the snippet has no sense at all so we are wasting scroll space. We should have something to decide if we want to show the snippet or not.

I'm not sure about that one. I'll ask @arturbosch to provide his expertise.

On the other hand I copy&pasted some html code from the android linter. I don't know how to point that out in the code.

I don't quite know what you mean by that. Could you please explain that?

This implemetation is really basic. It creates things like this:

Does your report create html files similar to the provided screenshot?

BraisGabin · 2019-10-04T21:23:59Z

That's fine for version 1. I think you meant syntax coloring, right. There's probably a library that does syntax coloring for Kotlin.

Ok, let's keep this out of this PR

The file writing process happens in the abstract base class called OutputReport. The derived class HtmlOutputReport should not implement this for testability reasons.

I didn't explain myself properly. The problem is that the Findings don't contain the snippet that I need (and that's ok). So I need to open the file to read the data that I need. Each Finding has the file path so I can do it. My point is that we should inject something to OutputReport to open this files. Something similar as: https://android.googlesource.com/platform/tools/base/+/master/lint/cli/src/main/java/com/android/tools/lint/HtmlReporter.java#93

What do you think?

Does your report create html files similar to the provided screenshot?

Yes, it's a screenshot of a detekt report with this PR. And that screenshot is a bit old. Now it doesn't underline the spaces. And with #1977 it will not underline the , neither

BraisGabin · 2019-10-05T12:58:04Z

CI should pass as soon as #1971 is merged

schalkms · 2019-10-05T17:13:51Z

CI should pass as soon as #1971 is merged

No, it won't necessarily, since CodeFactor updates detekt's dependencies on their own.
Unfortunately, you have to fix the issue by your own for now.

arturbosch · 2019-10-06T16:05:31Z

Cool PR, thanks!
Regarding opening the file again and deciding for rules to print the location or not:
What rules do you have in might which should not print the code snippet?

For not opening the file:
A Finding has an entity.ktElement which is nullable. KtElement.containingFile would return the KtFile which has the text method. This could bring the desired text content.
If entity.ktElement is per se a KtFile that must be like a formatting rule or something ike MaxLineLength. I do not know by hard if KtElement can actually be null now or if we report everytime a KtElement.
However the reporting logic should handle the null case.

BraisGabin · 2019-10-06T18:38:27Z

🎉 I don't need to open files anymore!

Examples of rules where the snippet is not usefull:

TooManyFunctions
ComplexMethod
ComplexInterface
LargeClass
NestedBlockDepth
...

I was thinking about doing something with the lenght of textLocation. If it's larger than X don't show the snippet but we would have some false positives. For example ClassNaming. The error is only the name, so the snippet is helpful but the finding show ALL the class as an error.

Maybe we should ignore this for now and try to fix the textLocation. The problem of fixig this is that we can break some baselines... Any idea?

schalkms

Looks good!
The productive code looks fine to me.
I'd just move the FlowContent util functions into a separate file, since they do quite a lot.
Please see my comments regarding the tests below.

detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/out/HtmlOutputReport.kt

schalkms · 2019-10-07T20:33:40Z

detekt-cli/src/test/kotlin/io/gitlab/arturbosch/detekt/cli/out/HtmlOutputFormatTest.kt

+        val code = """
+            package cases
+            // reports 1 - line with just one space
+
+            // reports 1 - a comment with trailing space
+            // A comment
+            // reports 1
+            class TrailingWhitespacePositive {
+                // reports 1 - line with just one tab
+
+                // reports 1
+                fun myFunction() {
+                    // reports 1 - line with 1 trailing tab
+                    println("A message")
+                // reports 1
+                }
+            }
+        """.trimIndent().splitToSequence('\n')


These test code tests too much. It's hard to maintain.
I think for testing the HTML code snippet, 1 or 2 violations should be sufficient.

schalkms · 2019-10-07T20:37:55Z

detekt-cli/src/test/kotlin/io/gitlab/arturbosch/detekt/cli/out/HtmlOutputFormatTest.kt

+            assertThat(snippet).isEqualTo("""
+                <div>
+                  <pre><code><span class="lineno">   4 </span>// reports 1 - a comment with trailing space
+                <span class="lineno">   5 </span>// A comment
+                <span class="lineno">   6 </span>// reports 1
+                <span class="lineno">   7 </span><span class="error">class TrailingWhitespacePositive {</span>
+                <span class="lineno">   8 </span>    // reports 1 - line with just one tab
+                <span class="lineno">   9 </span>
+                <span class="lineno">  10 </span>    // reports 1
+                </code></pre>
+                </div>
+
+                """.trimIndent()


These asserts test more too much. If we add 1 or 2 violations in the snippet, this and the following asserts should become easier to read and maintain.

I'm not sure if I understood you. The production code writes one snippet for each finding (fidnig = violation?) so it's impossible to add multiple violations to one single snippet.

Anyway, I agree that the problem of this test is the maintainability. I changed the sample source code onces and I need to change the line numbers in all the assertions. But I don't like the tests that are in this file already. They test nearly nothing. If you, or anyone, has an idea to simplify them but not losing its "power" I would like to improve them.

@BraisGabin As mentioned, I think 1 integration test that assert the dynamic (generated) parts of the HTML report, could be a good addition to this test group.
So the test checks character by character if the dynamic generated content matches the expected one. The expected string could be put into the resources folder in the src/test package.
Does this answer your question?

I think so. So you say that this tests instead of adding all that """code""" just create files in the resources folder and check that the output of the function is the same as the content in those files. Right?

Add add an integration test too.

No, this code is fine here. The val code = "..." should be simplified to contain 2 issues. Then the asserts also becomes much simpler by checking only the HTML content for these 2 issues.
The separate resource is just for the integration test that asserts the full HTML content.

val core = "..." contains 0 errors right now. I have all those tests because they test different cases:

Check that the underline is correct:

Underline all the line

Underline part of the line

Underline multiple lines

Show the correct lines:

If the error is in the first line I show lines 1 to 4.

If the error is in the first line I show lines 1 to 5.

If the error is in the line 7 I show lines 4 to 10 (important to show the change between one digit and 2 digits to be sure that the alignment works, this one is checked)

If the error is in the penultimate line I show lines 14 to 16.

If the error is in the last line I show lines 13 to 16.

The case 3 is checked implicitly in the top ones.

Maybe the ones that check the lines that are showed can be simplified to just: contains("1", "2", "3", "4"). Would that work?

detekt-cli/src/test/kotlin/io/gitlab/arturbosch/detekt/cli/out/HtmlOutputFormatTest.kt

BraisGabin · 2019-10-10T09:07:02Z

Move the snippet code outside HtmlOutputFormat and I created an intrumentation test for HtmlOutputFormatTest. I'm not a big fan of the intrumentation tests... this test can break SO easily...

I was thinking about creating a Fake of KtElement and PsiFile so I could have a more scoped test but... That would be a lot of code doing nothing. What do you prefer?

BraisGabin · 2019-10-11T11:37:35Z

Ok, I changed the integration test. Now I'm mocking the KtElement so this test will no broke so often.

By my side this PR is ready. I have ideas for future improvements but I think that this is OK as first step.

Are the tests now ok for you?

arturbosch · 2019-10-15T18:35:41Z

I don't need to open files anymore!

Examples of rules where the snippet is not usefull:

TooManyFunctions

ComplexMethod

ComplexInterface

LargeClass

NestedBlockDepth

...

I was thinking about doing something with the lenght of textLocation. If it's larger than X don't show the snippet but we would have some false positives. For example ClassNaming. The error is only the name, so the snippet is helpful but the finding show ALL the class as an error.

Maybe we should ignore this for now and try to fix the textLocation. The problem of fixig this is that we can break some baselines... Any idea?

Sorry, for the late reply. For findings concerning a function or class I think we would do the user a favor to print just the function and class headers.

class A(d: D) : B, C by d and fun largeFunction(....)

This should be useful?

BraisGabin · 2019-10-15T19:26:54Z

Sorry, for the late reply. For findings concerning a function or class I think we would do the user a favor to print just the function and class headers.

class A(d: D) : B, C by d and fun largeFunction(....)

This should be useful?

I agree. In that case we can move that work to other PRs fixing the TextLocation. So this PR is ready to be merged.

arturbosch · 2019-10-15T19:45:26Z

I will just wait for @schalkms last review :)

schalkms

As mentioned, we should consider a mocking library for testing detekt.
Furthermore, I think the unit tests could be more fine-grained.

CC @arturbosch

schalkms · 2019-10-16T14:39:29Z

detekt-cli/src/test/kotlin/io/gitlab/arturbosch/detekt/cli/out/FakePsiFile.kt

+import org.jetbrains.kotlin.com.intellij.psi.search.SearchScope
+import javax.swing.Icon
+
+class FakePsiFile(


We should consider a mocking library here. This becomes really really hard to maintain.

schalkms · 2019-10-16T14:43:11Z

detekt-cli/src/test/kotlin/io/gitlab/arturbosch/detekt/cli/out/HtmlUtilsTest.kt

+        val code = """
+            package cases
+            // reports 1 - line with just one space
+
+            // reports 1 - a comment with trailing space
+            // A comment
+            // reports 1
+            class TrailingWhitespacePositive {
+                // reports 1 - line with just one tab
+
+                // reports 1
+                fun myFunction() {
+                    // reports 1 - line with 1 trailing tab
+                    println("A message")
+                // reports 1
+                }
+            }
+        """.trimIndent().splitToSequence('\n')


Continued discussion from above:

val core = "..." contains 0 errors right now. I have all those tests because they test different cases:

Check that the underline is correct:

Underline all the line
Underline part of the line
Underline multiple lines
Show the correct lines:

If the error is in the first line I show lines 1 to 4.
If the error is in the first line I show lines 1 to 5.
If the error is in the line 7 I show lines 4 to 10 (important to show the change between one digit and 2 digits to be sure that the alignment works, this one is checked)
If the error is in the penultimate line I show lines 14 to 16.
If the error is in the last line I show lines 13 to 16.
The case 3 is checked implicitly in the top ones.

Maybe the ones that check the lines that are showed can be simplified to just: contains("1", "2", "3", "4"). Would that work?

Yes, please try to split this test up as good as possible. For testing the full HTML reporter we have the integration test.

schalkms · 2019-10-16T14:43:34Z

detekt-cli/src/test/kotlin/io/gitlab/arturbosch/detekt/cli/out/HtmlOutputFormatTest.kt

            assertThat(result).contains("<span class=\"description\">A3</span>")
        }
+
+        it("integration") {


nit: Please provide a more detailed description here.

BraisGabin · 2019-10-16T16:21:01Z

I simplified the tests that were related with the number of lines. The others I insist that they are needed. They were necessary to develop the feature.

codecov-io · 2019-10-16T17:28:08Z

Codecov Report

❗ No coverage uploaded for pull request base (master@d907933). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #1975   +/-   ##
=========================================
  Coverage          ?   80.84%           
  Complexity        ?     1989           
=========================================
  Files             ?      330           
  Lines             ?     5622           
  Branches          ?     1025           
=========================================
  Hits              ?     4545           
  Misses            ?      537           
  Partials          ?      540

Impacted Files	Coverage Δ	Complexity Δ
...tlab/arturbosch/detekt/cli/out/HtmlOutputReport.kt	`90.38% <100%> (ø)`	`13 <0> (?)`
...n/io/gitlab/arturbosch/detekt/cli/out/HtmlUtils.kt	`100% <100%> (ø)`	`0 <0> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d907933...30097e6. Read the comment docs.

schalkms

Thanks! I already like this feature. 🥇🙂

This removes some boilerplate code from the HtmlOuputFormatTest introduced by #1975.

* Rename it to finding * Add snippet code in html report * Add instegration test * Improve integration test * Simplify tests * Rename test

This removes some boilerplate code from the HtmlOuputFormatTest introduced by detekt#1975.

* Rename it to finding * Add snippet code in html report * Add instegration test * Improve integration test * Simplify tests * Rename test

This removes some boilerplate code from the HtmlOuputFormatTest introduced by detekt#1975.

BraisGabin mentioned this pull request Oct 4, 2019

Fix EnumNaming textLocation #1977

Merged

BraisGabin mentioned this pull request Oct 6, 2019

Allow the user to collapse the rules in the html report #1997

Merged

BraisGabin changed the title ~~Add snippet code in web report~~ Add snippet code in html report Oct 6, 2019

schalkms reviewed Oct 7, 2019

View reviewed changes

BraisGabin requested a review from schalkms October 10, 2019 09:07

BraisGabin added 3 commits October 11, 2019 00:05

Rename it to finding

ae76c63

Add snippet code in html report

f433403

Add instegration test

70cd5f8

Improve integration test

c3bbc7f

arturbosch approved these changes Oct 15, 2019

View reviewed changes

arturbosch added this to the 1.2.0 milestone Oct 15, 2019

schalkms reviewed Oct 16, 2019

View reviewed changes

BraisGabin added 2 commits October 16, 2019 18:17

Simplify tests

9bcdc56

Rename test

30097e6

BraisGabin requested a review from schalkms October 16, 2019 16:19

schalkms approved these changes Oct 16, 2019

View reviewed changes

arturbosch merged commit d31e162 into detekt:master Oct 24, 2019

BraisGabin deleted the web-report branch October 24, 2019 14:15

schalkms mentioned this pull request Oct 26, 2019

Introduces mocking library 'mockk' #2055

Merged

arturbosch pushed a commit that referenced this pull request Oct 27, 2019

Introduces mocking library 'mockk' (#2055)

675fd12

This removes some boilerplate code from the HtmlOuputFormatTest introduced by #1975.

smyachenkov pushed a commit to smyachenkov/detekt that referenced this pull request Dec 9, 2019

Add snippet code in html report (detekt#1975)

0f714a6

* Rename it to finding * Add snippet code in html report * Add instegration test * Improve integration test * Simplify tests * Rename test

smyachenkov pushed a commit to smyachenkov/detekt that referenced this pull request Dec 9, 2019

Introduces mocking library 'mockk' (detekt#2055)

1743f0f

This removes some boilerplate code from the HtmlOuputFormatTest introduced by detekt#1975.

smyachenkov pushed a commit to smyachenkov/detekt that referenced this pull request Dec 9, 2019

Add snippet code in html report (detekt#1975)

27f11b1

* Rename it to finding * Add snippet code in html report * Add instegration test * Improve integration test * Simplify tests * Rename test

smyachenkov pushed a commit to smyachenkov/detekt that referenced this pull request Dec 9, 2019

Introduces mocking library 'mockk' (detekt#2055)

838babe

This removes some boilerplate code from the HtmlOuputFormatTest introduced by detekt#1975.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add snippet code in html report #1975

Add snippet code in html report #1975

BraisGabin commented Oct 3, 2019 •

edited

Loading

schalkms commented Oct 4, 2019

BraisGabin commented Oct 4, 2019

BraisGabin commented Oct 5, 2019

schalkms commented Oct 5, 2019

arturbosch commented Oct 6, 2019

BraisGabin commented Oct 6, 2019 •

edited

Loading

schalkms left a comment

schalkms Oct 7, 2019

schalkms Oct 7, 2019

BraisGabin Oct 8, 2019

schalkms Oct 8, 2019

BraisGabin Oct 8, 2019

schalkms Oct 9, 2019 •

edited

Loading

BraisGabin Oct 10, 2019

BraisGabin commented Oct 10, 2019

BraisGabin commented Oct 11, 2019

arturbosch commented Oct 15, 2019

BraisGabin commented Oct 15, 2019

arturbosch commented Oct 15, 2019

schalkms left a comment

schalkms Oct 16, 2019

schalkms Oct 16, 2019

schalkms Oct 16, 2019

BraisGabin commented Oct 16, 2019 •

edited

Loading

codecov-io commented Oct 16, 2019

schalkms left a comment

Add snippet code in html report #1975

Add snippet code in html report #1975

Conversation

BraisGabin commented Oct 3, 2019 • edited Loading

schalkms commented Oct 4, 2019

BraisGabin commented Oct 4, 2019

BraisGabin commented Oct 5, 2019

schalkms commented Oct 5, 2019

arturbosch commented Oct 6, 2019

BraisGabin commented Oct 6, 2019 • edited Loading

schalkms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schalkms Oct 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BraisGabin commented Oct 10, 2019

BraisGabin commented Oct 11, 2019

arturbosch commented Oct 15, 2019

BraisGabin commented Oct 15, 2019

arturbosch commented Oct 15, 2019

schalkms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BraisGabin commented Oct 16, 2019 • edited Loading

codecov-io commented Oct 16, 2019

Codecov Report

schalkms left a comment

Choose a reason for hiding this comment

BraisGabin commented Oct 3, 2019 •

edited

Loading

BraisGabin commented Oct 6, 2019 •

edited

Loading

schalkms Oct 9, 2019 •

edited

Loading

BraisGabin commented Oct 16, 2019 •

edited

Loading