Hi Ondra,

The extra assertMethodCallCount api is a great idea.

I'm mildly concerned that the change in the printed  message format (to include the extra text "Method MyClass#myMethod") breaks backwards compatibility. However, the extra info is  i) purely additive and ii) very useful in the case where the input message arg is null. Since it is unlikely that there will be many dtest client tests which rely on strict parsing of assert failure messages I think this is low impact enough to merit your change.

I like the ability to specify the method list without having to use reflection in the JVM running dtest. Having to have the relevant jars available in both the dtest JVM and the JVM being traced can be awkward in some cases, especially when the jars are buried in a JBoss EAP module tree. There is, of course, a danger of uploading erroneous trace rules if you don't  use reflection to filter the proposed method set. These should just  be ignored by the agent.so they don't do any great harm (and if they are important then assertions in the test ought to catch that failure). However,  it would be better if the injection errors were detected.

Mind you the same criticism applies to the current code where failure to find a method via reflection is silently ignored. That  really ought to be fixed -- perhaps by throwing an exception in the dtest client to derail the test? (I'll try to remember to raise a JIRA against it). There is a JIRA in place to allow Submit clients (like dtest and BMUnit) to check for rules which have failed to inject or had errors at/after injection so it might be good to extend the dtest code to use this capability once it is available. That would certainly deal with any concern about adopting this new api and also address the case where an upload fails even though the reflection test passes which is possible when there is a version mismatch between the jar providing a class to the dtest JVM and the one loaded by the traced JVM.

Anyway, thanks very much for providing this very helpful  PR. I am afraid it is being flagged as having conflicts with the current code base. Would you be able to investigate that and rebase it against the latest tree or would you prefer me to pull into a local copy and then try to edit it myself?

regards,

Andrew Dinn

Ondřej Chaloupka wrote:

 

thanks for you comments. I did the rebase and the PR is now available to be merged.

Thanks. I'll pull it into trunk asap.

Ondřej Chaloupka wrote:

 

I was looking around and wonder if there is some unit tests for dtest library. I mean a "<=/>=" error could occur elsewhere

No, there are no such tests but a contribution is always welcome :-).

Of course, it is hard to test this properly using a standard maven setup because proper testing ought to employ a dtest jvm (i.e. the one started by maven to run, say, JUnit) plus a separate traced JVM. However, I guess some basic tests  which operated within a single JVM would still be helpful.

Are you using dtest a lot? If so can you point to a repo containing the tests and/or explain what they do? I have no real idea how many people are using this package (other than the TX tests that Jonathan Halliday wrote it to support). It might be interesting to have a link to some example tests that would get more users interested in trying it.