From da4e7ffb12001bc1e8a28cd87566de4c2c0e3c1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20M=C3=B6hn?= Date: Wed, 8 Aug 2018 08:56:32 +0900 Subject: [PATCH 1/3] WHITESPACE test_basic: Remove superfluous empty line I must have accidentally added it at some point. Remove it, because it is inconsistent with the rest of the file. --- tests/test_basic.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_basic.py b/tests/test_basic.py index c05ff4e..5e7012b 100644 --- a/tests/test_basic.py +++ b/tests/test_basic.py @@ -93,7 +93,6 @@ def testUnlockWorkspace(self): def testUnlockedLockedPointer(self): """Test whether root reply with an unlocked and a locked pointer works. """ - db = Datastore() sched = Scheduler(db) From 44eff84892140c7f77c0d4191c152aa51eef68e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20M=C3=B6hn?= Date: Wed, 8 Aug 2018 08:58:15 +0900 Subject: [PATCH 2/3] Context: Make __eq__ stricter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context.__eq__ was based on the string representation (str-rep) of the context. This makes sense for the memoizer, which should consider two contexts the same when they have the same str-rep, because it imitates H and H only ever sees the str-rep of contexts. However, this str-rep-based equality throws off other algorithms. See the test case that I added to test_basic. There we have two contexts with the same str-rep but different workspaces. Call them C1 and C2. sched.resolve_action adds both of them to sched.pending_contexts, such that: sched.pending_contexts = deque([C2, C1]) After the Reply("$a1 $a2"), sched.choose_context determines: choice = C1 Then it does sched.pending_contexts.remove(C1). However, str(C1) == str(C2) → C1 == C2, so what gets removed from sched.pending_contexts is C2; C1 remains. This is wrong and leads to failure shortly after. Fix this by making Context.__eq__ based on a context's workspace_link, unlocked_locations and parent. I chose these three attributes, because every context can be constructed from them. This should work in the general case, but it breaks the memoizer. So change the memoizer to explicitly stringify contexts before doing anything with them. Implemenation notes: - Change Context.__eq__ to return NotImplemented when `other` is not a Context, because that's what the Python docs recommend: https://docs.python.org/3/reference/datamodel.html#the-standard-type-hierarchy - Leave Context.__hash__ as it is, because hash(c1) == hash(c2) is allowed to happen occasionally. Also, `hash` has to be fast, so it makes sense to use the str-rep, which is already stored in the context. - The `str` calls inside Memoizer are repetitive, but I think it would be overkill to implement an abstraction. --- patchwork/context.py | 8 +++++--- patchwork/scheduling.py | 15 ++++++++++----- tests/test_basic.py | 24 ++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 8 deletions(-) diff --git a/patchwork/context.py b/patchwork/context.py index 01f79f8..8de667f 100644 --- a/patchwork/context.py +++ b/patchwork/context.py @@ -151,8 +151,10 @@ def __hash__(self) -> int: return hash(str(self)) def __eq__(self, other: object) -> bool: - if type(other) is not Context: - return False - return str(other) == str(self) + if not isinstance(other, Context): + return NotImplemented + return self.workspace_link == other.workspace_link \ + and self.unlocked_locations == other.unlocked_locations \ + and self.parent == other.parent diff --git a/patchwork/scheduling.py b/patchwork/scheduling.py index a45ece2..7fd9567 100644 --- a/patchwork/scheduling.py +++ b/patchwork/scheduling.py @@ -96,20 +96,25 @@ def handle(self, context: Context) -> Action: class Memoizer(Automator): + """A memoizer for H's actions. + + This memoizer learns H's mapping ``str(Context) → Action`` and can be called + instead of H for all string representations of contexts that it has seen. + """ def __init__(self): - self.cache: Dict[Context, Action] = {} + self.cache: Dict[str, Action] = {} def remember(self, context: Context, action: Action): - self.cache[context] = action + self.cache[str(context)] = action def forget(self, context: Context): - del self.cache[context] + del self.cache[str(context)] def can_handle(self, context: Context) -> bool: - return context in self.cache + return str(context) in self.cache def handle(self, context: Context) -> Action: - return self.cache[context] + return self.cache[str(context)] class Scheduler(object): diff --git a/tests/test_basic.py b/tests/test_basic.py index 5e7012b..e532377 100644 --- a/tests/test_basic.py +++ b/tests/test_basic.py @@ -99,3 +99,27 @@ def testUnlockedLockedPointer(self): with RootQuestionSession(sched, "Root?") as sess: sess.act(AskSubquestion("Sub1?")) sess.act(Reply("$q1 $a1")) + + + def testEqualStringRepresentation(self): + """Test processing of contexts with equal string representation. + + If two contexts have the same string representation that includes a + locked pointer, make sure: + + * That none of the contexts gets dropped. + * That when a reply is given in the first context, the + :py:class:`patchwork.Memoizer` correctly outputs the same reply in + the second context. + """ + db = Datastore() + sched = Scheduler(db) + + with RootQuestionSession(sched, "Bicycle Repair Man, but how?") as sess: + sess.act(AskSubquestion("Is it a [stockbroker]?")) + sess.act(AskSubquestion("Is it a [quantity surveyor]?")) + sess.act(Reply("$a1 $a2")) + sess.act(Reply("NO! It's Bicycle Repair Man.")) + self.assertEqual("[[NO! It's Bicycle Repair Man.]" + " [NO! It's Bicycle Repair Man.]]", + sess.root_answer) From 2136b022f8eea2191926b48a9cdd1f26f4c0b189 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Richard=20M=C3=B6hn?= Date: Tue, 14 Aug 2018 07:56:18 +0900 Subject: [PATCH 3/3] Add test_randomly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For exercising Patchwork with random inputs. Implementation notes: - I don't know if the code involving threading is correct. - Leave many TODOs, because doing them would good, but isn't crucial for the random test to be useful. - Some identifiers have a trailing underscore (hypothesis_) in order to avoid shadowing global variables. - One might think that starting a thread for every act() is slow, but on my machine, which is old, it increases the running time of the random test by only a few seconds (7 s → 8-12 s). - Someone in https://stackoverflow.com/questions/2829329/ advises to catch BaseException (instead of Exception) and re-raise in (Terminable)Thread.join. Don't do that, because a SystemExit in a child thread shouldn't terminate the parent, in my opinion. - Custom exception types are good. Cf. https://www.youtube.com/watch?v=wf-BqAjZb8M. --- dev_requirements.txt | 1 + tests/terminable_thread.py | 93 ++++++++++++++++ tests/test_randomly.py | 213 +++++++++++++++++++++++++++++++++++++ 3 files changed, 307 insertions(+) create mode 100644 tests/terminable_thread.py create mode 100644 tests/test_randomly.py diff --git a/dev_requirements.txt b/dev_requirements.txt index e77189f..248153c 100644 --- a/dev_requirements.txt +++ b/dev_requirements.txt @@ -1 +1,2 @@ mypy==0.600 +hypothesis==3.67.0 diff --git a/tests/terminable_thread.py b/tests/terminable_thread.py new file mode 100644 index 0000000..facb430 --- /dev/null +++ b/tests/terminable_thread.py @@ -0,0 +1,93 @@ +"""Defines a thread that can be terminated from the outside. + +Main class: :py:class:`TerminableThread` + +Based on: https://stackoverflow.com/a/325528/5091738 +Which in turn is based on: http://tomerfiliba.com/recipes/Thread2/ +Mixed in: https://stackoverflow.com/questions/2829329/ +""" + +import ctypes +import sys +import threading + + +class ThreadExit(Exception): + """Injected into the thread in order to make it terminate.""" + pass + +class TerminationError(Exception): + """Raised when a thread keeps running even after injecting :py:exc:`ThreadExit`. + """ + pass + + +class TerminableThread(threading.Thread): + """Like :py:class:`threading.Thread`, but can be killed from the outside. + + The semantics are the same as those of :py:class:`threading.Thread`, except + that the method :py:meth:`TerminableThread.terminate` is added and if the + child thread raised an exception, :py:meth:`TerminableThread.join` will + re-raise it. + """ + + def __init__(self, *args, **kwargs): + super(TerminableThread, self).__init__(*args, **kwargs) + self.exc_info = None + self.result = None + self.terminated = False + + + def run(self): + try: + if self._target: + self.result = self._target(*self._args, **self._kwargs) + except ThreadExit: + self.terminated = True + except Exception: + self.exc_info = sys.exc_info() + finally: + # Avoid a refcycle if the thread is running a function with + # an argument that has a member that points to the thread. + # Credits: threading source + del self._target, self._args, self._kwargs + + + def join(self, timeout=None): + super(TerminableThread, self).join(timeout) + if self.exc_info: + raise self.exc_info[1] + return self.result + + + def _raise_exception(self, exc_type: type): + """Raise exception with the given type in the thread represented by self. + + If the thread is waiting for a system call (eg. ``time.sleep()``, + ``socket.accept()``) to return, it will ignore the exception. + """ + lident = ctypes.c_long(self.ident) + res = ctypes.pythonapi.PyThreadState_SetAsyncExc( + lident, + ctypes.py_object(exc_type)) + + if res == 0: + raise ValueError("PyThreadState_SetAsyncExc failed to find {}." + .format(self)) + + if res != 1: + # "if it returns a number greater than one, you're in trouble, + # and you should call it again with exc=NULL to revert the effect" + # (https://svn.python.org/projects/stackless/Python-2.4.3/dev/Python/pystate.c) + ctypes.pythonapi.PyThreadState_SetAsyncExc(lident, None) + raise SystemError("PyThreadState_SetAsyncExc failed for {}." + .format(self)) + + + def terminate(self, timeout=0.01): + self._raise_exception(ThreadExit) + if timeout is not None: + self.join(timeout) + if self.is_alive(): + raise TerminationError("Attempted to terminate {}, but it keeps" + " running.".format(self)) diff --git a/tests/test_randomly.py b/tests/test_randomly.py new file mode 100644 index 0000000..98e0966 --- /dev/null +++ b/tests/test_randomly.py @@ -0,0 +1,213 @@ +"""Property-based test for Patchwork. + +Main class: :py:class:`PatchworkStateMachine` + +Also defines :py:class:`TestRandomly`, which unit testing tools can detect. +""" + +import logging +import re +import sys +from typing import Any, List, Optional + +import hypothesis.strategies as st +from hypothesis.searchstrategy import SearchStrategy +from hypothesis.stateful import RuleBasedStateMachine, precondition, rule + +from patchwork.actions import Action, AskSubquestion, Reply, Scratch, Unlock +from patchwork.context import Context +from patchwork.datastore import Datastore +from patchwork.scheduling import RootQuestionSession, Scheduler +from tests import terminable_thread + +logging.basicConfig(level=logging.INFO) + + +# Strategies ############################################### + +# A strategy for generating hypertext without any pointers, ie. just text. +# +# Notes: +# - [ and ] are for expanded pointers and $ starts a locked pointer. Currently +# there is no way of escaping them, so filter them out. +# - min_size=1, because if we allow empty arguments to actions, infinite +# loops become more likely, which slows down test execution. +ht_text = st.text(min_size=1).filter(lambda t: re.search(r"[\[\]$]", t) is None) + + +def expanded_pointer(hypertext_: SearchStrategy[str]) -> SearchStrategy[str]: + """Turns a strategy generating hypertext into one generating pointers to it. + + An expanded pointer to hypertext "" has the form "[]". + """ + return hypertext_.map(lambda ht_: "[{}]".format(ht_)) + + +def join(l: List[str]) -> str: + return "".join(l) + + +def hypertext(pointers: List[str]) -> SearchStrategy[str]: + """Return a strategy for generating nested hypertext. + + The resulting strategy can generate any hypertext. Ie. a mix of text, + unexpanded pointers, and expanded pointers containing hypertext. The + resulting string won't have whitespace at either end in order to be + consistent with command-line Patchwork, which strips inputs before passing + them to the :py:class:`Action` initializers. + + Parameters + ---------- + pointers + Unexpanded pointers that the strategy may put in the resulting + hypertext. + + Returns + ------- + A strategy that generates hypertext. The generated hypertext will be empty + sometimes. + """ + protected_pointers = st.sampled_from(pointers).map(lambda p: p + " ") + # Protected from being garbled by following text: $1␣non-pointer text + leaves = st.lists(ht_text | protected_pointers, min_size=1).map(join) + # Concerning min_size see the comment above ht_text. + return st.recursive( + leaves, + lambda subtree: st.lists(subtree | expanded_pointer(subtree), + min_size=1).map(join) + ).map(lambda s: s.strip()) + + +def question_hypertext(pointers: List[str]) -> SearchStrategy[str]: + """Return a strategy for generating hypertext that is suited for questions. + + Some hypertexts are likely to create infinite loops when used as a + subquestion. See issue #15. This procedure returns the same strategy as + :py:func:`hypertext`, except that it filters out those unsuitable cases. + + See also + -------- + :py:func:`hypertext` + """ + return hypertext(pointers).filter(lambda s: not s.startswith("[")) + + +# Test case ################################################ + +class PatchworkStateMachine(RuleBasedStateMachine): + """Bombard patchwork with random questions, replies etc. + + This test doesn't contain any assertions, yet, but at least it makes sure + that no action will cause an unexpected exception. + """ + def __init__(self): + super(PatchworkStateMachine, self).__init__() + self.db: Optional[Datastore] = None + self.sess: Optional[RootQuestionSession] = None + + + @property + def context(self) -> Context: + return self.sess.current_context + + + def pointers(self) -> List[str]: + """Return a list of the pointers available in the current context.""" + return list(self.context.name_pointers_for_workspace( + self.context.workspace_link, self.db) + .keys()) + + + def locked_pointers(self) -> List[str]: + return [pointer + for pointer, address in self.context.name_pointers.items() + if address not in self.context.unlocked_locations] + + + # TODO: Make the waiting time for the join adaptive. + # TODO: If we call t.terminate() after the thread has finished (because + # it finished just after the timeout), we get an error. Avoid or catch that. + def act(self, action: Action): + t = terminable_thread.TerminableThread(target=lambda: self.sess.act(action), + name="Killable Session.act", + daemon=False) + t.start() + waiting_time = 0.5 + t.join(waiting_time) + + if t.is_alive(): + logging.info("Terminating the execution of action %s, because it" + " might be caught in an infinite loop (execution time" + " > %s s).", action, waiting_time) + try: + t.terminate() + except terminable_thread.TerminationError: + sys.exit("Couldn't kill the thread that is executing action {}." + " Aborting in order to avoid system overload." + .format(action)) + + + # TODO generation: Make sure that sometimes a question that was asked + # before is asked again, also in ask(), so that the memoizer is exercised. + # TODO assertion: The root answer is available immediately iff it was in + # the datastore already. + @precondition(lambda self: not self.sess) + @rule(data=st.data(), + is_reset_db=st.booleans()) + def start_session(self, data: SearchStrategy[Any], is_reset_db: bool): + if self.db is None or is_reset_db: + self.db = Datastore() + self.sess = RootQuestionSession( + Scheduler(self.db), + question=data.draw(question_hypertext([]))) + if self.sess.root_answer: + self.sess.__exit__() + self.sess = None + + + @precondition(lambda self: self.sess) + @rule(data=st.data()) + def reply(self, data: SearchStrategy[Any]): + self.act( + Reply(data.draw(hypertext(self.pointers())))) + if self.sess.root_answer: + self.sess.__exit__() + self.sess = None + + + # TODO assertion: Unlocking already unlocked pointers causes an exception. + @precondition(lambda self: self.sess) + @rule(data=st.data()) + def unlock(self, data: SearchStrategy[Any]): + self.act( + Unlock(data.draw(st.sampled_from(self.locked_pointers())))) + + + # TODO: Move the try-except to self.act(), because the infinite loop + # error can also occur with other actions. Cf. + # https://github.com/oughtinc/patchwork/issues/12#issuecomment-404002144. + # TODO assertion: Infinite loop error is only thrown when it should be + # thrown. + @precondition(lambda self: self.sess) + @rule(data=st.data()) + def ask(self, data: SearchStrategy[Any]): + question = data.draw(question_hypertext(self.pointers())) + try: + self.act( + AskSubquestion(question)) + except ValueError as e: + if "Action resulted in an infinite loop" not in str(e): + raise + + + @precondition(lambda self: self.sess) + @rule(data=st.data()) + def scratch(self, data: SearchStrategy[Any]): + self.act( + Scratch(data.draw(hypertext(self.pointers())))) + + +# Runner ################################################### + +TestRandomly = PatchworkStateMachine.TestCase