search

atomist-attic / rug

DEPRECATED Runtime for Rugs
GNU General Public License v3.0
53 stars 13 forks source link

Cannot test command handlers that use the path expression engine with cortex #613

Closed ddgenome closed 6 years ago

ddgenome commented 7 years ago

You cannot currently test command handlers whose handle method uses the path expression engine from the context and accesses cortex nodes.

Why? Currently the only method to add nodes to the root context in a test is addToRootContext:

addToRootContext(n: GraphNode): void;

As you can see, it allows you to add the node, but not the axis/relationship between the added node and the root node. This works fine for adding projects to the root node because in projects we can traverse child, i.e., any, axes. In cortex, we require named axes when stepping from one node to the next. Thus, when going from a command handler's root node, which is a ChatTeam to a ChatChannel, you must include the axis in the path expression: /channels::ChatChannel(). Since you cannot specify the axes, or edge in path expression engine speak, when the path expression engine evaluates the path expression against the root node, it finds no matches since the channels edge does not appear in the list of edges for the root node due to failing the axis test.

https://github.com/atomist/rug/blob/f6a37092938ea01683bf771694402dd8593ef769/src/main/scala/com/atomist/graph/GraphNode.scala#L50-L67

I was thinking of adding an optional second argument for addRootContext to allow specifying the "edge" connecting the node being added to the root node. What do people think?

/cc @johnsonr @whostolebenfrog

whostolebenfrog commented 7 years ago

This makes a lot of sense. It can definitely be a non-breaking change which is always nice. I can't see any reason why we wouldn't want to allow this - I don't think it complicates the API.

So +1 from me - I can't think of a nicer way to handle this.

johnsonr commented 7 years ago

I hadn't actually read this issue, as I hadn't been in rug lately. :-( My bad. However, i added a setContextRoot method yesterday as this problem proved a blocker on what I was doing. This is now being used successfully in spring-team-handlers.

I didn't change the behavior of the existing method. However, I wonder if there is any value in addToRootContext. The root context is always a ChatTeam. It should be possible in BDD testing to add a stub (pure JS) implementation, and even named edge support might create issues in JS/JVM interop.

The implementation of the set method simply replaces the context root the user passes in. The user can then retrieve that node (which should be ChatTeam) and add to it in subsequent set up. When path expressions are run, that node is automatically converted to a JVM node by the existing jsPathExpressionEngine implementation.