Skip to content

Commit

Permalink
Tests & fixes for story change mid session bug. (#27)
Browse files Browse the repository at this point in the history
Tests & fixes for story change mid session bug.
  • Loading branch information
rabidgremlin authored Aug 29, 2019
1 parent bcdfce5 commit 9ce04e7
Show file tree
Hide file tree
Showing 10 changed files with 275 additions and 10 deletions.
2 changes: 1 addition & 1 deletion gradle.properties
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
version=5.0.3
version=5.0.4-SNAPSHOT

# These are place holder values. Real values should be set in user's home gradle.properties
# and are only needed when signing and uploading to central maven repo
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,9 +203,9 @@ public BotResponse respond(Session session, Context context, String messageText)

// call hook so externs and other things can be applied
afterStoryCreated(story);

// restore the story state
SessionUtils.loadInkStoryState(session, story.getState());
SessionUtils.loadInkStoryState(session, story, inkStoryJson);

// call hook so additional things can be applied to story after state has been restored
afterStoryStateLoaded(story);
Expand Down Expand Up @@ -318,7 +318,7 @@ public BotResponse respond(Session session, Context context, String messageText)
SessionUtils.setFailedToUnderstandCount(session, failedToUnderstandCount);

// save current story state
SessionUtils.saveInkStoryState(session, story.getState());
SessionUtils.saveInkStoryState(session, story, inkStoryJson);

// does story have any more choices ?
if (story.getCurrentChoices().size() == 0)
Expand Down Expand Up @@ -414,6 +414,22 @@ private StringBuffer processStory(Session session, CurrentResponse currentRespon
while (story.canContinue())
{
String line = story.Continue();

// log any warnings
// in theory we should be getting a warning if state wasn't restored correctly
// so we can handle the issue. this is currently not happening.
// See TestSessionRestore for current hack
if (story.hasWarning())
{
log.warn("Ink story has warnings: {}", story.getCurrentWarnings());
}

// log any errors
if (story.hasError())
{
log.error("Ink story has errors: {}", story.getCurrentErrors());
}

processStoryLine(line,response,currentResponse, session, intentMatch, story);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.rabidgremlin.mutters.bot.ink;

import com.bladecoder.ink.runtime.Story;
import com.bladecoder.ink.runtime.StoryState;
import com.rabidgremlin.mutters.core.session.Session;

Expand Down Expand Up @@ -53,10 +54,13 @@ public static int getFailedToUnderstandCount(Session session)
* Stores the current Ink story state for the user into the session.
*
* @param session The session.
* @param storyState The story state.
* @param story The story.
* @param storyJson The JSON string for the story. Used to temporary story version check hack. See TestSessionRestore.
*/
public static void saveInkStoryState(Session session, StoryState storyState)
{
public static void saveInkStoryState(Session session, Story story, String storyJson)
{
StoryState storyState = story.getState();

if (storyState == null)
{
throw new BadInkStoryState("storyState should not be null");
Expand All @@ -65,6 +69,8 @@ public static void saveInkStoryState(Session session, StoryState storyState)
try
{
session.setAttribute(SLOT_PREFIX + "0987654321STORYSTATE1234567890", storyState.toJson());
// save length of story JSON and use as a crude version check
session.setAttribute(SLOT_PREFIX + "0987654321STORYJSONLENGTH1234567890", Integer.valueOf(storyJson.length()));
}
catch (Exception e)
{
Expand All @@ -76,16 +82,28 @@ public static void saveInkStoryState(Session session, StoryState storyState)
* Gets the user's current Ink story state from the session.
*
* @param session The session.
* @param storyState The story state.
* @param story The story.
* @param storyJson The JSON string for the story. Used to temporary story version check hack. See TestSessionRestore.
*/
public static void loadInkStoryState(Session session, StoryState storyState)
public static void loadInkStoryState(Session session, Story story, String storyJson)
{
StoryState storyState = story.getState();

try
{
Integer storyJsonLength = (Integer) session
.getAttribute(SLOT_PREFIX + "0987654321STORYJSONLENGTH1234567890");
String stateJson = (String) session
.getAttribute(SLOT_PREFIX + "0987654321STORYSTATE1234567890");
.getAttribute(SLOT_PREFIX + "0987654321STORYSTATE1234567890");

if (stateJson != null)
{
// have state to restore so check that we have a story length and then check for size mismatch
if (storyJsonLength == null || storyJsonLength != storyJson.length())
{
throw new Exception("Story size mismatch. Assume new story has been loaded. Cannot restore session.");
}

storyState.loadJson(stateJson);
}
}
Expand Down
39 changes: 39 additions & 0 deletions mutters-ink-bot/src/test/ink/sessionrestore/story_new_option.ink
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
VAR conversation_has_completed_at_least_once = false

-> start


=== start ====
-> all_intents

= all_intents
+ [OneIntent] -> option_one
+ [NewIntent] -> option_new
+ [TwoIntent] -> option_two
+ [ThreeIntent] -> option_three

= option_one
You chose option one
-> restart_conversation

= option_two
You chose option two
+ [OneIntent] -> option_one
+ [TwoIntent] -> option_two
-> restart_conversation

= option_new
You chose option new
+ [OneIntent] -> option_one
+ [ThreeIntent] -> option_three
-> restart_conversation

= option_three
You chose option three
-> restart_conversation


=== restart_conversation ===
~ conversation_has_completed_at_least_once = true
-> start

Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
VAR conversation_has_completed_at_least_once = false

-> start


=== start ====
-> all_intents

= all_intents
+ [OneIntent] -> option_one
+ [TwoIntent] -> option_two
+ [ThreeIntent] -> option_three

= option_one
You chose option one
-> restart_conversation

= option_two
You chose option two
+ [OneIntent] -> option_one
+ [TwoIntent] -> option_two
-> restart_conversation

= option_three
You chose option three
-> restart_conversation


=== restart_conversation ===
~ conversation_has_completed_at_least_once = true
-> start
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package com.rabidgremlin.mutters.bot.ink;

public class SessionRestoreTestBot
extends InkBot<SessionRestoreTestBotConfiguration>
{

public SessionRestoreTestBot(SessionRestoreTestBotConfiguration config)
{
super(config);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
package com.rabidgremlin.mutters.bot.ink;

import java.util.List;

import com.rabidgremlin.mutters.core.IntentMatcher;
import com.rabidgremlin.mutters.templated.SimpleTokenizer;
import com.rabidgremlin.mutters.templated.TemplatedIntent;
import com.rabidgremlin.mutters.templated.TemplatedIntentMatcher;

public class SessionRestoreTestBotConfiguration implements InkBotConfiguration
{
private String inkJsonFileName;

public SessionRestoreTestBotConfiguration(String inkJsonFileName)
{
this.inkJsonFileName = inkJsonFileName;
}

@Override
public IntentMatcher getIntentMatcher()
{
SimpleTokenizer tokenizer = new SimpleTokenizer();

TemplatedIntentMatcher matcher = new TemplatedIntentMatcher(tokenizer);

TemplatedIntent oneIntent = matcher.addIntent("OneIntent");
oneIntent.addUtterance("one");

TemplatedIntent twoIntent = matcher.addIntent("TwoIntent");
twoIntent.addUtterance("two");

TemplatedIntent threeIntent = matcher.addIntent("ThreeIntent");
threeIntent.addUtterance("three");

TemplatedIntent newIntent = matcher.addIntent("NewIntent");
newIntent.addUtterance("new");

return matcher;
}

@Override
public String getStoryJson()
{
return StoryUtils.loadStoryJsonFromClassPath(inkJsonFileName);
}

@Override
public List<InkBotFunction> getInkFunctions()
{
// TODO Auto-generated method stub
return null;
}

@Override
public List<GlobalIntent> getGlobalIntents()
{
// TODO Auto-generated method stub
return null;
}

@Override
public ConfusedKnot getConfusedKnot()
{
// TODO Auto-generated method stub
return null;
}

@Override
public List<String> getDefaultResponses()
{
// TODO Auto-generated method stub
return null;
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
package com.rabidgremlin.mutters.bot.ink;

import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.CoreMatchers.notNullValue;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.fail;

import org.junit.BeforeClass;
import org.junit.Test;

import com.rabidgremlin.mutters.core.Context;
import com.rabidgremlin.mutters.core.bot.BotException;
import com.rabidgremlin.mutters.core.bot.BotResponse;
import com.rabidgremlin.mutters.core.session.Session;

/** Test session restores when story changes (which can happen if sessions are stored outside of the bot for instance in a external cache).
*
* Ink tries to gracefully handle story state restores when a story structure has changed. It is supposed to raise warnings and errors that can
* be checked when this occurs, but this is not happening. There is also a case where restore causes wrong path to be followed and incorrect output
* to be generated. See testSessionRestoreWhenStoryOptionsChange()
*
* Current 'hack' is treat any change of story as an incompatible state change and raise a BadInkStoryState.
*
*/
public class TestSessionRestore
{
private static SessionRestoreTestBot storyThreeBot;
private static SessionRestoreTestBot storyNewBot;

@BeforeClass
public static void setUpBot()
{
storyThreeBot = new SessionRestoreTestBot(new SessionRestoreTestBotConfiguration("story_three_options.ink.json"));
storyNewBot = new SessionRestoreTestBot(new SessionRestoreTestBotConfiguration("story_new_option.ink.json"));
}

@Test
public void testSessionRestoreWhenStoryOptionsChange() throws BotException
{
Session session = new Session();
Context context = new Context();

BotResponse response = storyThreeBot.respond(session, context, "Three");
assertThat(response.getResponse(), is("You chose option three"));

response = storyThreeBot.respond(session, context, "Three");
assertThat(response.getResponse(), is("You chose option three"));

response = storyThreeBot.respond(session, context, "Three");
assertThat(response.getResponse(), is("You chose option three"));

response = storyThreeBot.respond(session, context, "Three");
assertThat(response.getResponse(), is("You chose option three"));

// switch to new story with more options and change of options order
// Response should be "You chose option three" but actually land up "You chose option two" due to bug in ink
// so short term, we will just spit out a BadInkStoryState exception
try
{
response = storyNewBot.respond(session, context, "Three");
//assertThat(response.getResponse(), is("You chose option three"));
fail("Code should not reach here. Expected exception to be thrown");
}
catch(Exception e)
{
if (!e.getCause().getClass().equals(BadInkStoryState.class))
{
fail("Was expecting cause to be BadInkStoryState exception.");
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"inkVersion":19,"root":[[{"->":"start"},["done",{"#f":7,"#n":"g-0"}],null],"done",{"start":[{"->":".^.all_intents"},{"all_intents":[["ev","str","^OneIntent","/str","/ev",{"*":".^.c-0","flg":4},"ev","str","^NewIntent","/str","/ev",{"*":".^.c-1","flg":4},"ev","str","^TwoIntent","/str","/ev",{"*":".^.c-2","flg":4},"ev","str","^ThreeIntent","/str","/ev",{"*":".^.c-3","flg":4},{"c-0":["^ ",{"->":"start.option_one"},"\n",{"#f":7}],"c-1":["^ ",{"->":"start.option_new"},"\n",{"#f":7}],"c-2":["^ ",{"->":"start.option_two"},"\n",{"#f":7}],"c-3":["^ ",{"->":"start.option_three"},"\n",{"#f":7}]}],{"#f":3}],"option_one":["^You chose option one","\n",{"->":"restart_conversation"},{"#f":3}],"option_two":[["^You chose option two","\n","ev","str","^OneIntent","/str","/ev",{"*":".^.c-0","flg":4},"ev","str","^TwoIntent","/str","/ev",{"*":".^.c-1","flg":4},{"c-0":["^ ",{"->":"start.option_one"},"\n",{"#f":7}],"c-1":["^ ",{"->":".^.^.^"},"\n",{"->":"restart_conversation"},{"#f":7}]}],{"#f":3}],"option_new":[["^You chose option new","\n","ev","str","^OneIntent","/str","/ev",{"*":".^.c-0","flg":4},"ev","str","^ThreeIntent","/str","/ev",{"*":".^.c-1","flg":4},{"c-0":["^ ",{"->":"start.option_one"},"\n",{"#f":7}],"c-1":["^ ",{"->":"start.option_three"},"\n",{"->":"restart_conversation"},{"#f":7}]}],{"#f":3}],"option_three":["^You chose option three","\n",{"->":"restart_conversation"},{"#f":3}],"#f":3}],"restart_conversation":["ev",1,"/ev",{"temp=":"conversation_has_completed_at_least_once","re":true},{"->":"start"},{"#f":3}],"global decl":["ev",0,{"VAR=":"conversation_has_completed_at_least_once"},"/ev","end",null],"#f":3}],"listDefs":{}}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"inkVersion":19,"root":[[{"->":"start"},["done",{"#f":7,"#n":"g-0"}],null],"done",{"start":[{"->":".^.all_intents"},{"all_intents":[["ev","str","^OneIntent","/str","/ev",{"*":".^.c-0","flg":4},"ev","str","^TwoIntent","/str","/ev",{"*":".^.c-1","flg":4},"ev","str","^ThreeIntent","/str","/ev",{"*":".^.c-2","flg":4},{"c-0":["^ ",{"->":"start.option_one"},"\n",{"#f":7}],"c-1":["^ ",{"->":"start.option_two"},"\n",{"#f":7}],"c-2":["^ ",{"->":"start.option_three"},"\n",{"#f":7}]}],{"#f":3}],"option_one":["^You chose option one","\n",{"->":"restart_conversation"},{"#f":3}],"option_two":[["^You chose option two","\n","ev","str","^OneIntent","/str","/ev",{"*":".^.c-0","flg":4},"ev","str","^TwoIntent","/str","/ev",{"*":".^.c-1","flg":4},{"c-0":["^ ",{"->":"start.option_one"},"\n",{"#f":7}],"c-1":["^ ",{"->":".^.^.^"},"\n",{"->":"restart_conversation"},{"#f":7}]}],{"#f":3}],"option_three":["^You chose option three","\n",{"->":"restart_conversation"},{"#f":3}],"#f":3}],"restart_conversation":["ev",1,"/ev",{"temp=":"conversation_has_completed_at_least_once","re":true},{"->":"start"},{"#f":3}],"global decl":["ev",0,{"VAR=":"conversation_has_completed_at_least_once"},"/ev","end",null],"#f":3}],"listDefs":{}}

0 comments on commit 9ce04e7

Please sign in to comment.