Proposal of dtest library enhancement

[ochaloup]

Hi all,

I would like propose an enhancement to dtest library API. My initial PR is here: https://github.com/bytemanproject/byteman/pull/37

What are points what I'm trying to enhance

• adding "api" methods assertMethodCallCount to InstrumentedClass
  • this idea came from the fact how I use the instrumented class assertion. I normally check numer of precise method calls and this is a shortcut for not necessarily need to create new CallCount instance in my code
• change of assertion info in InstrumentedInstance#assertMethodCallCount. I use method assertMethodCalled really quite often and I'm still a lot confused of the message that the method assertMethodCallCount shows. This is enhancement that assertion message will be more comprehensible - saying what class and method is in account
• adding method instrumentClass under Instrumentor. The point here is that it accept class name as a string and not just a class. I do not see any point to restrict this on class as the class name is then changed to string at the end. This would be quite handy for me  as time to time I do not want to need to depend on client on a library that the remote server side contains and I want to hook on

What do you think could be this PR accepted?

Thank you

Ondra

[adinn]

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

[ochaloup]

Hi Andrew,

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

I'm happy that you are not strictly against changing assertion message of InstrumentedInstance#assertMethodCallCount.as I was a bit worried about your care of backward compatibility

And I'm looking forward to not necessarily need to add dependencies of JBoss EAP module tree to my tests.

That's interesting point that you raised here. I mean that currently finding a method via reflection is silently ignored. I was wondering several times why nothing happens and later on find out that I put down wrong class or method name. From my user point of view it seems to me as good idea for extension.

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

[adinn]

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.

[ochaloup]

Sure, I see

I use dtest quite progressively but it's part of our internal EAP integration testsuite testing transactions that is unfortunately hard to be linked. Maybe I would try to write down some introductory documentation. If I come with something I let you know.