search

WinterTechForum / skybar

Skybar: Live code coverage engine
11 stars 3 forks source link

Added support for inner classes by allowing *class names* with '$'s, … #18

Closed jackgene closed 9 years ago

jackgene commented 9 years ago

Added support for inner classes by filtering out only source file names with '$' in them (which generally indicates generated sources), instead of filtering out any class name with '$' in them.

In other words, if a class name has '$' in it, but its source file does not have '$' in it, we want to instrument it.

Note that I tried updating tests to verify this case, but the testing framework wasn't quite handling inner classes correctly. I'll have a look at this later.

eirbjo commented 9 years ago

Jack,

The filtering is done on the className in the form given by ClassFileTransformer.transform:

com/skybar/demo/LongLoopServlet$1

For performance reasons, this is done before any byte code parsing. So we don't know anything about the source file name when making this decision.

Also note that the code you changed in shouldInstrument was excluding class names containing "$$", that's a double $. So I don't think we were skipping inner classes at all.

In fact, the demo source has a LongLoopServlet with an anon inner class which is instrumented and counted just fine.

What do you think?

eirbjo commented 9 years ago

Jack,

This might just be me not knowing my way around Github, but wouldn't it be a tad easier if you made these changes in a branch on WinterTechForum/skybar? That way I would not have to clone your fork for testing and we could contribute together on the same branch.

This is the way @marshallpierce and I have been working and it just seems a bit easier.

jackgene commented 9 years ago

Let me look into the inner class thing. It wasn't working for me for some reason.

As for just using branches on this repository, yup, that works for me, I can move my changes here as well.

jackgene commented 9 years ago

Hey Eirik, I'm going to be on vacation next week, but here's what I found last week. I think I was trying to solve two problems with this:

  1. When using Java beans annotated with Jackson annotations for JSON serialization, they generate Java classes with a single '$' in them, hence making it through the filter (this is not why inner classes doesn't work though, it just pollutes the UI).
  2. The reason inner classes wasn't showing up was a Scala-specific problem. Scala generates class names with '$$' in them when generating inner classes (from functions etc), and these inner classes aren't getting instrumented at all.