Two classes with the same simple name confuse scoverage

[RichardBradley]

If a project has two classes with the same simple name in different packages, then the front page of the report will have only one line for that class, and the stats reported on that line will be the aggregate of the two classes. The link will go to the file report for the first of the two classes.

There will be two separate per-file reports correctly generated for the two classes, but they will only be available via the package links.

(This seems pretty minor to me; and it's not actually causing me problems, as we only had two classes with the same simple name due to an oversight, but I thought I'd document it here.)

[RichardBradley]

This is now causing me more problems: I have multiple inner classes with the same name, and scoverage is misattributing uncovered lines in all of them to the first:

File "my/stuff/Foo.scala":

package my.stuff

class Foo {
 ...
}

object Foo {
  class Request {
    ...
  }
}

File "my/stuff/Bar.scala":

package my.stuff

class Bar {
 ...
}

object Bar {
  class Request {
    ...
  }
}

... scoverage groups MeasuredLines by Location.class (in coverage.scala:57), which distinguishes only by the class simple name ("Request" in this case), and not by the full class name (Foo.Request).

(The property "Location.fqn" is also bugged in the above case, reporting my.stuff.Request when it should be my.stuff.Foo.Request)

I'll see if I can get a PR together to fix.

[RichardBradley]

This happens even if the ambiguous name is "$anon", which might be a more sympathetic case than multiple inner classes all named "Request" :-)

I think some DSLs like Spray Routing will trigger this, as they create inner classes named "$anon". If you have two such inner classes in the same package belonging to two different outer classes, it will trigger this bug.

[gslowikowski]

Hi Richard
I looked at this problem. At first sight using Location.fqn for grouping seems the proper way to go. As you already found, fqn does not return proper full qualified class name. There is not enough information in scoverage.coverage.xml file to construct one. Maybe full internal class name should be added there. @sksamuel, WDYT ?
But, after some thinking, I don't see much sense in grouping by and showing all internal classes. Top level class name will be enough IMO.
What do you think about changing fqn function definition to:

  val fqn = (packageName + ".").replace("<empty>.", "") + topLevelClass

(and using it for grouping, of course). With this change only top level classes would be shown in overview.html report.

Additionally, can you create sample project with Spray Routing generated classes you mentioned?

[RichardBradley]

I think some DSLs like Spray Routing will trigger this,

can you create sample project with Spray Routing generated classes you mentioned?

After some further investigation, it seems that Spray's DSL is not involved here. Sorry for the confusion.
The repro I found involves explicit anonymous classes, e.g.
val myObj = new { ... } with SomeTrait

I have uploaded a repro case to https://github.com/RichardBradley/scoverage-27-demo

Scoverage is correctly filtering out anonymous classes which are created to capture closures (e.g. for DSLs), but it reports explicit anonymous classes in the report with name "$anon". (Perhaps that is a separate bug?)

If you have two classes in the same package which both use explicit anonymous classes, then the two classes will have the same simple name but different FQNs, so they will trigger this bug as described at the top of this thread.

[gslowikowski]

I've updated checked out https://github.com/RichardBradley/scoverage-27-demo, updated sbt-scoverage plugin version to 1.0.4 and run coverage.
On the overview.html report page I have three classes: Class1, Class2 and NamedObject. I see no problems here.

[RichardBradley]

You're right, the "$anon" issue for anonymous classes is fixed in the latest version. Please ignore my previous two comments :-)

I have updated the example to demo this issue for inner classes which have the same name instead -- see RichardBradley/scoverage-27-demo@6b0bcf2

[RichardBradley]

Perhaps listing "classes" in the report view is asking for trouble. Given that the coverage tool is "line" based, it might be simpler to list "files" in that report view instead? Then we could just ignore all this complexity around anon classes, implied classes for lambdas etc. etc.

[RichardBradley]

(I really need to update to the latest scoverage version on our big commercial Scala project. I've been putting it off as we wrote quite a bit of custom SBT logic to add scoverage to our "functional" (integration) tests, and it will all need redoing.)

[sksamuel]

The latest builds of sbt-scoverage are miles better for playing nicely with
SBT. You don't need to be as hacky now as it will play with whatever custom
scopes you made (it, func tests etc). The downside was that you have to do
a separate report step sbt scoverageReport

On 10 April 2015 at 11:16, Richard Bradley [email protected] wrote:

(I really need to update to the latest scoverage version on our big
commercial Scala project. I've been putting it off as we wrote quite a bit
of custom SBT logic to add scoverage to our "functional" tests, and it will
all need redoing.)

—
Reply to this email directly or view it on GitHub
#27 (comment)
.

[gslowikowski]

I've tested your modified project. As I commented earlier (#27 (comment)) because Location.fqn value is not proper for internal classes I would change it from:

  val fqn = (packageName + ".").replace("<empty>.", "") + className

to

  val fqn = (packageName + ".").replace("<empty>.", "") + topLevelClass

and use for grouping in ClassBuilders.classes. Alternatively we can add (and use) another val instead of changing fqn implementation (how would we name it?).
WDYT?

[sksamuel]

What you said makes sense, either that or enclosingClass. It depends on how we want to handle nested classes.

So

class Foo {
   class Boo {
       // function here
  }
}

do we want that function to be grouped with packageName.Foo.Boo or packageName.Foo or packageName.Boo

[gslowikowski]

1. packageName.Foo.Boo - what about internal classes nested inside other internal classes? e.g. we have two internal classes: packageName.Foo.Woo.Boo and packageName.Foo.Moo.Boo, how do you distinguish the two different Boo classes? It's impossible co construct proper fqn for them now.
2. packageName.Foo reasonable and doable now,
3. packageName.Boo doesn't make sense,

[gslowikowski]

@sksamuel, what about this? If you have no objections I will change fqn implementation to

val fqn = (packageName + ".").replace("<empty>.", "") + topLevelClass

and release 1.1.0 (not 1.0.5, we discussed it) versions of scalac-scoverage-plugin and sbt-scoverage.

[sksamuel]

I think we should not use the topLevelClass but the full package +
enclosing path.

Otherwise:

object Foo {
object Boo {
object Moo
}
object Goo {
object Moo
}
}

The moos would clash.

On 17 April 2015 at 09:44, Grzegorz Slowikowski [email protected]
wrote:

@sksamuel https://github.com/sksamuel, what about this? If you have no
objections I will change fqn implementation to

val fqn = (packageName + ".").replace(".", "") + topLevelClass

and release 1.1.0 (not 1.0.5, we discussed it) versions of
scalac-scoverage-plugin and sbt-scoverage.

—
Reply to this email directly or view it on GitHub
#27 (comment)
.

[gslowikowski]

There is no full enclosing path stored in scoverage.coverage.xml now. Will you add it?

[sksamuel]

Of course, we can add whatever we want to the xml (just not remove things
:))

I think if we store the full path, the top level type, and the filepath,
then we've got all bases covered.

On 17 April 2015 at 09:57, Grzegorz Slowikowski [email protected]
wrote:

There is no full enclosing path stored in scoverage.coverage.xml now.
Will you add it?

—
Reply to this email directly or view it on GitHub
#27 (comment)
.

[sksamuel]

I can do this tonight actually if you'd prefer me to do it ?

[gslowikowski]

Yes, I don't know Scalac internals.

[gslowikowski]

I would like to fix this issue and release 1.1.0 ASAP because users are asking for it.

[sksamuel]

Yep I can get to around 5pm UK time, shouldn't take long.

On 17 April 2015 at 10:12, Grzegorz Slowikowski [email protected]
wrote:

I would like to fix this issue and release 1.1.0 ASAP because users are
asking for it.

—
Reply to this email directly or view it on GitHub
#27 (comment)
.

[gslowikowski]

Ping

[sksamuel]

@gslowikowski I wasn't able to do this as I didn't have the gpg keys with me while away on business in Washington, which I thought I had . I've released it now.

[gslowikowski]

Why you've closed this issue? It's not fixed.

[sksamuel]

Overzealous.

[gslowikowski]

See my comment to merged PR

[RichardBradley]

Thanks, @gslowikowski - that's very useful.

This should be an easy fix; are you able to create a PR for it?

If not, I'll try to fix in the next few days (I don't have Cobertura or anything which expects Cobertura formatted XML output, so I'm not able to test that this actually works, but your comments are specific enough that I can create a fix on that basis).

I have created #146 to track this, as it's a separate issue to this one, and this one is now closed.

[gslowikowski]

Unfortunately I have no time. I have test project, but cannot attach zip file to the issue (not supported file type).

[gslowikowski]

I found one more bug, with method name of nested class.

Source:

package a.b.c

object Foo
{
  def hello: String = "Hello Foo"

  object Boo
  {
    def hello: String = "Hello Foo.Boo"

    object Moo
    {
      def hello: String = "Hello Foo.Boo.Moo"
    }
  }
}

Fragment of scoverage.xml file:

                <class 
                name="a.b.c.Foo.Boo.Moo" filename="a\b\c\Foo.scala" statement-count="1" statements-invoked="1" statement-rate="100.00" branch-rate="100.00">
                    <methods>
                        <method name="a.b.c/Moo/hello" statement-count="1" statements-invoked="1" statement-rate="100.00" branch-rate="100.00">
                            <statements>
                                <statement 
                                package="a.b.c" class="Moo" class-type="Object" full-class-name="a.b.c.Foo.Boo.Moo" source="C:\my\windows\path\to\the\project\src\main\scala\a\b\c\Foo.scala" method="hello" start="180" end="199" line="13" branch="false" invocation-count="1" ignored="false">
</statement>
                            </statements>
                        </method>
                    </methods>
</class>

1
Method name is a.b.c/Moo/hello now. Should be a.b.c/Foo.Boo.Moo/hello, or something more natural - a.b.c.Foo.Boo.Moo.hello (class name is a.b.c.Foo.Boo.Moo, so why method name should use slashes as separators?)

2
Statement class is Moo, should be Foo.Boo.Moo (the same problem as above).

[RichardBradley]

gslowikowski reopened this 3 days ago

(I'm closing this again, as I think this issue "Two classes with the same simple name confuse scoverage" is now fixed and we are now discussing "Cobertura XML output format doesn't exactly match real cobertura format for class names, method names" which is tracked at #146)

[gslowikowski]

In my comment above I'm writing about scoverage.xml, not cobertura.xml.