[WFCORE-7043] Subsystem-level testing of core-managment is incomplete #6235
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
| <process-state-listener name="x" class="org.acme.foo.MyClass" module="org.acme.foo" timeout="${process.timeout:5000}"> | ||
| <properties> | ||
| <property name="foo" value="true"/> | ||
| <property name="bar" value="${bar.prop:2}"/> | ||
| </properties> | ||
| </process-state-listener> |
There was a problem hiding this comment.
@lvydra You also need to add the test case that reads the subsystem with the expression resolved and assert that the values resolved are the expected ones.
| private Object getValue(ModelNode node, String attributeName) { | ||
| ModelNode result = node.get(attributeName).resolve(); | ||
| return result.asString(); | ||
| } |
There was a problem hiding this comment.
Reviewing this I've just learnt that Elytron tests (e.g ResolveExpressionAttributesTestCase) use the same approach you did here. However, this is a weak test. What we are testing here is just how the ModelNode resolves expressions.
What I was expecting instead is to read the resource with the expressions resolved so we can getin the operation result the expressions resolved by the server code, hence we will be testing the expression resolution of the read operation with this subsystem instead of just testing that the ModelNode object is able to resolve an expression.
@lvydra Do you follow me on this and do you agree? For your info, I've found this example of what we should be testing here https://github.com/wildfly/wildfly/blob/main/naming/src/test/java/org/jboss/as/naming/subsystem/NamingSubsystemTestCase.java#L113-L117
I'm going to open a Jira for ResolveExpressionAttributesTestCase since in my opinion it should be hardened testing the server code expression resolution and not the ModelNode object.
There was a problem hiding this comment.
@yersan Now that I see it, I have to agree. I'll look into it.
|
Thanks @lvydra for taking on this JIRA. I wasn't asking that we validate the expression resolution in the individual test. We just need to ensure that in the files we use for the executions of AbstractSubsystemBaseTest.standardSubsystemTest for every schema version and every attribute we use an expression (if the attribute is of a nature that should support expressions). This is just a simple good practice to help guard against mistakes. I think adding logic to validate expression resolution beyond whatever already happens in standardSubsystemTest should be a separate task, and done as shared logic in AbstractSubsystemBaseTest or some similar base thing. Individual subsystem tests shouldn't need to deal with it directly. NOTE: I'm not asking anyone to do that; I'm just looking for the addition of expressions so what's already there is exercised and others looking at the files get a sense of the general best practice. NOTE: if this PR has a working validation in CoreManagementSubsystemTestCase, I'm not suggesting it be removed. ^^^ is only relevant if getting the validation done is blocking things. |
There was a problem hiding this comment.
I think it would have been better to assert a value resolved instead of getting the default if doesn't exist, but it should be ok as it is. Following Brian's note, expression resolution in individual tests should not block this, so I'm approving this from my side. Thanks @lvydra
This is just a simple good practice to help guard against mistakes.
@bstansberry ok, so following your comment, my understanding is that the inclusion of these expressions in the XML subsystem stanza under test is just to verify that the attributes that accept expressions can be parsed with expressions and that we will get a consistent behaviour preventing that an attribute that currently accepts an expression is not going to change this behavior in future releases.
Just out of curiosity, is there any other reason behind this reasoning?
Right now AbstractSubsystemBaseTest.standardSubsystemTest does not exercise a real expression resolution (as far as I know). I think it could be tricky to make it so generic as to get it under AbstractSubsystemBaseTest without any intervention passing the expected attributes and values from the child class.
yersan
added
the
ready-for-merge
|
Yes, all I'm asking for is that the xml uses expressions. Doing more is fine if people have time but I was not asking for that. |
|
@yersan @lvydra Further to this... Have a look at AttributeDefinition.validateAndSet. It...
If the attribute def doesn't support expressions, but the xml uses one and the attribute type is one where an arbitrary string can't convert to the target ModelType, this will fail. So boolean attributes and numerics or complex objects with elements fields like that. So if someone forgets to support expressions on a string attribute, this won't catch it, but for booleans/numerics it will. The real point of this "best practice" is to encourage devs to mindlessly follow the pattern of using expressions everywhere and thereby hopefully catch mistakes when they use if for things where they forgot to add expression support. |
|
Thanks @lvydra and @bstansberry for the explanations and review |
https://issues.redhat.com/browse/WFCORE-7043