Re: test for default history content

On 29-03-14 20:26, Zjnue Brzavi wrote:
> On Sat, Mar 29, 2014 at 1:24 PM, Jim Barnett <1jhbarnett@gmail.com
> <mailto:1jhbarnett@gmail.com>> wrote:
>
>     Zjnue,
>       I agree that we need more tests, but why would defaultHistoryContent need
>     to get passed to getTargetStates?  It's just an accessor function.  Though I
>     do think we have to consider how getTransitionDomain and findLCCA should
>     handle history states. They ignore them at the moment, and _maybe_ that's ok...
>
>     - Jim
>
>
> Hi Jim,
>
> Perhaps as quick background, my implementation follows (atm) the proposed
> algorithm pretty much verbatim, in order to aid debugging for cases like this
> and to make getting started with SCXML a little easier for myself and others.
>
> Simply put, the recently published changes to the algorithm for
> defaultHistoryContent support are not satisfying test579.
>
> While the getTargetStates function seems pretty harmless, it is also
> hisory-aware and can redirect a transition target.

Hi Zjnue,

Sorry for chiming in this late, and going back to this earlier message, but I'm 
puzzled by the above statement.

I can find no indication in the algorithm nor in the specification text that the 
getTargetStates function is supposed to do anything more than just returning the 
direct target(s) of a transition.

It seems to me that the problems you encountered are actually *caused* by the 
the history-awareness and dereferencing in your getTargetStates implementation.

AFAICT the pseudo algorithm expects it will take care of the history 
dereferencing itself in the addDescendantStatesToEnter procedure.
If you do this upfront in you getTargetStates implementation, the 
addDescendantStatesToEnter never will get to 'see' a history state as it is 
supposed to, and neither can it then handle the default history content as just 
was added.

I just completed the implementation of the current algorithm for Apache Commons 
SCXML (although not yet committed), and I now can successfully run test579, so 
it seems to me the proposed change simply works as expected.

But maybe I'm overlooking some other specification requirements for getTargetStates?
I'd appreciate some pointers in that case :)

Regards, Ate


> In the case of test579, the <initial> transition, targets <history id="sh1">,
> but is effectively redirected to <state id="s01" >, skipping any executable
> content of <history> along the way.
>
> Please take a look at my code for getTargetStates and getTargetState below [1].
>
> It is only with the following two lines present in getTargetState:
>                      if( defaultHistoryContent != null && h.parent.exists("id") )
>                          defaultHistoryContent.set(h.parent.get("id"),
> h.transition().next());
> ..that 579 passes.
>
> As mentioned, I did not look at it in depth and will not be able to spend more
> time on it over the next few days unfortunately.
>
> So in summary, after adding the published additions to the algorithm, test579
> did not pass.
> It was only after the changes to getTargetStates that it did.
> Hope this helps for now.
>
> Thanks,
> Zjnue
>
> [1]
>
>      function getTargetStates( node : Node, ?defaultHistoryContent :
> Hash<Iterable<Node>> ) : List<Node> {
>          var l = new List<Node>();
>          if( !node.exists("target") )
>              return l;
>          var ids = node.get("target").split(" ");
>          var top = node;
>          while( top.parent != null && !top.isTScxml() )
>              top = top.parent;
>          for( id in ids ) {
>              var ts = getTargetState(top, id, defaultHistoryContent);
>              for( tss in ts )
>                  l.add( tss );
>          }
>          return l;
>      }
>
>      function getTargetState( s : Node, id : String, ?defaultHistoryContent :
> Hash<Iterable<Node>> ) : List<Node> {
>          if( s.get("id") == id )
>              return [s].toList();
>          else {
>              for( child in s.getChildStates() ) {
>                  var ss = getTargetState(child, id, defaultHistoryContent);
>                  if( ss != null )
>                      return ss;
>              }
>              for( h in s.history() ) {
>                  var hh = getTargetState(h, id);
>                  if( hh != null ) {
>                      if( defaultHistoryContent != null && h.parent.exists("id") )
>                          defaultHistoryContent.set(h.parent.get("id"),
> h.transition().next());
>                      return historyValue.exists( h.get("id") ) ?
>                          historyValue.get( h.get("id") ) :
>                          getTargetStates( h.transition().next(),
> defaultHistoryContent );
>                  }
>              }
>          }
>          return null;
>      }
>
>
>
>     On 3/28/2014 8:51 PM, Zjnue Brzavi wrote:
>>
>>         I've added test579 to the suite.  Let me know if there are problems
>>         with it or if you think there need to be more/different tests.--
>>
>>
>>     Hi,
>>
>>     Not had a very good look yet, but it appears we'll need to pass
>>     defaultHistoryContent to getTargetStates as well, as it is another area
>>     where the algorithm is skipping past some history elements without
>>     considering their executable content. This diff passes test579 :
>>     https://github.com/zjnue/hscxml/commit/4b9091470d3c7c1995e6b41900535e0437ec3eb4
>>     Another test or 2 on the subject is probably a good idea.
>>
>>     Best regards,
>>     Zjnue
>
>     --
>     Jim Barnett
>     Genesys
>
>

Received on Tuesday, 1 April 2014 18:45:44 UTC