Wiring tests and modifications

[porcherg]

I modified this method:

createWireContext(String wireDefinitionXml)

this method now check that the parsing generates no problems (some problems in parsing were forgotten)

I've added the following tests:

testDefaultConstructorWithWrongArgs

test the error raised when no constructor matching the given arguments is found.

testNonStaticFactoryMethod

test the error raised on a call of a non static method on a null object.

testAutoWireWrongType

test auto wiring when a field name match with an object in the context but the types are not assignable.


I have also corrected ObjectBinding to only search for operations in elements with an operation tagName (without that, constructors and factory elements generated a "no binding found" error during parsing)

Guillaume

[porcherg]

I will add tests for LongDescriptor in BasicTypeWireTests.

I have also modified the LongBinding file to parse elements with the same behavior as other number bindings (catch the NumberFormatException and add this to the list of parse problems)

Guillaume

[tom.baeyens]

cool. thanks.

i added the list of problems in case a problem is detected.

i'm not sure, but we might have to add scanning for errors in the problems.

the idea is that problems relate to problems in the IDE (e.g. the Problems pane in eclipse). but as in eclipse, the problems pane contains errors and warnings.

failing when there is a problem might be inconvenient in case the parsing generates a warning that you want to ignore in that test case.

that doesn't have to be fixed urgently, but i just wanted you to know the background

[porcherg]

in ObjectDescriptor, I modified the getArgs method.

If the ArgDescriptor specifies a type, we verify that the type of the created argument is compatible. If there is an incompatibility, a WireException is thrown.

Before this change, no check was performed, and a JbpmReflectException was thrown when invoking a method with bad args.

Guillaume

[tom.baeyens]

i removed the check.

the purpose of the arg types is not fir checking, but for method matching. also it wouldn't have worked with null values (but that could have been fixed).

the exception will come later, when the argument is being used. i don't think an explicit check is good cause there will always could be situations where the check fails and you actually knew what you were doing.

it would be good if you could add test cases that check what kind of exception message you get, and wether they are informative.

i improved the exception messages for arguments a bit with an indication of which argument gives the problem

[porcherg]

it would be good if you could add test cases that check what kind of exception message you get, and wether they are informative.
i improved the exception messages for arguments a bit with an indication of which argument gives the problem

In DependencyTest, in case of circular dependency, the error message is now "couldn't create a: couldn't create argument 0". Before the change, it was "circular dependency for a". I think it gave more information to the user.

The exception with the informative message is now encapsulated in many other exceptions (depending on the size of the dependency cycle). I don't think concatenating all the exceptions message would be a better solution.

Is there another solution ?

Regards,
Guillaume

[tom.baeyens]

"porcherg" wrote:
In DependencyTest, in case of circular dependency, the error message is now "couldn't create a: couldn't create argument 0". Before the change, it was "circular dependency for a". I think it gave more information to the user.

you're right. it's fixed

"porcherg" wrote:
The exception with the informative message is now encapsulated in many other exceptions (depending on the size of the dependency cycle). I don't think concatenating all the exceptions message would be a better solution.

yes. i do think that is a good solution. in case of such exceptions, you can immediately see the dependency cycle.

you can make your life easier if you can analyst the full problem from a stack trace that your supoort-paying customers send to you :-)

[porcherg]

"porcherg" wrote:
I have also corrected ObjectBinding to only search for operations in elements with an operation tagName (without that, constructors and factory elements generated a "no binding found" error during parsing)

I modified this again to only filter arg, factory and contructor elements. With the previous version, the test

public void testBadOperation(){
 List<Problem> problems = parseProblems(
 "<objects>" +
 " <object name='o' class='foo'>" +
 " <bad-operation/>" +
 " </object>" +
 "</objects>"
 );
 assertNotNull(problems);
 }

failed because no problems were generated during parse. I modified the class to make the test work.

I added more checks (constructor elements with factories now generate an error)

Guillaume

[tom.baeyens]

i'm not so sure yet that generating problems is a good thing.

as long as you create problems in the parse, it is probably ok.

but in case you generate problems for bad XML here, you should do it everywhere. so you should have a good xsd and check all kinds of problems.

keep in mind that people might want to extend the parser to parse additional attributes and elements.

with all that in mind. i think it's better not to generate a problem for it, WDYT ?

[porcherg]

as long as you create problems in the parse, it is probably ok.

I just added problems in the parse (no exception thrown)

keep in mind that people might want to extend the parser to parse additional attributes and elements.
with all that in mind. i think it's better not to generate a problem for it, WDYT ?

I am not sure I have well understood, so I will try to explain my point of view.

In the current version, if we add new elements (extensions), which are not in the category 'operation', they will be ignored by the parser.
I think a warning that explains that the element couldn't be parsed or what are the allowed tags in the ObjectBinding is better than just ignoring the element and give no information to the user (it allows him for example to easily fix typos, or if it is not a mistake, he can just ignore the message).

If the user adds a binding to the parser in the category "operation", the new element will be parsed correctly and there will be no more warning in the parse.

There are a lot of place in the wire xml where there are the same kind of restrictions on the elements (for example "map" element can only contain "entry" elements).

In any case, a bad tag during the parsing will just generate parse problems and no exception. Then, if the result of the parsing is invalid, there will be a WireException thrown when the objects are created from the descriptors.

Is that what you had in mind or do you think it is better to remove the parse problem ?

Guillaume

[porcherg]

I have added some tests for the wiring context.

The EnlistTest tests are not working because the resource is enlisted twice (see http://www.jboss.com/index.html?module=bb&op=viewtopic&t=116285 )

Guillaume

[porcherg]

I added a ContextTest test class, that is testing some methods available in WireContext such as clear the cache, remove an object from the cache...

These tests showed a problem with the clear() method (but I think it's fixed).

Guillaume