Rework basic tests and update a number of explicit tests #820

marcharper · 2017-01-16T06:58:34Z

I'm simplifying #807 by porting out some of the changes. This PR mostly contains updates to the testing suite including:

Reworked some history internals to give more representative simulations
TestHeadsUp is now TestMatch. We should expand and encourage testing actual matches and their outcomes.
markov_test([C, D, D, C]) is now second_play_test(C, D, D, C) (name is debatable)
responses_test got some upgrades: the argument order is now responses_test(responses, history1=None, history2=None, ...) so you can test the first X actions with e.g. responses_test([C,D,D,...]) without any other arguments
Some tests were rewritten
Many PEP8 and docstring fixes.
Some updates to the testing docs

You may note some additional warnings now in the test suite -- if the requested history of play is impossible then a warning will be raised (but the test can still pass). This is to encourage the writing of better tests. We can suppress these.

…gs for mismatched histories

marcharper · 2017-01-16T20:33:19Z

There's one doc build test failing that I'm not sure how to fix.

drvinceknight · 2017-01-16T20:56:03Z

@marcharper, I've just pushed 81853d6 to this branch (I hope that's ok). I believe that should fix your rest syntax errors.

drvinceknight

This all looks great @marcharper, I have a couple of requests about the documentation where I feel things could be tightened up a bit as well as possibly changing history1, history2 as variables in responses_test to be player_history, opponent_history (or perhaps the other way around: I'm not entirely sure).

When this goes it it will close #726

drvinceknight · 2017-01-17T09:26:09Z

axelrod/mock_player.py

        # Need to retain history for opponents that examine opponents history
        # Do a deep copy just to be safe
        super().__init__()
-        self.history = copy.deepcopy(player.history)


Why do we not need this?

The new code below it copies the list element by element using update_history.

Cool. Thanks.

drvinceknight · 2017-01-17T09:26:36Z

axelrod/mock_player.py



-def simulate_play(P1, P2, h1=None, h2=None):
+def simulate_play(player1, player2, h1=None, h2=None):


history1, history2 perhaps?

I changed h1 and h2 to action1 and action2.

drvinceknight · 2017-01-17T09:40:32Z

axelrod/tests/unit/test_cooperator.py

@@ -43,13 +39,11 @@ class TestTrickyCooperator(TestPlayer):
    }

    def test_strategy(self):
-        """Starts by cooperating."""
+        # Starts by cooperating.


Why an inline comment instead of a docstring?

Because the docstring doesn't fully describe the function and a lot of others were this way, so I standardized them. We can write better doc strings for these if you want but it's essentially always the same thing. The inline comments are better since they explain what each group of assertions is testing.

I'm indifferent, although in terms of PEP8 standards is a docstring not what is usual here?

Sure but the docstring is either generic for all instances of test_strategy, i.e. """Tests that the strategy returns the expected actions.""" or it's essentially a repeat of the strategy's docstring. Is there a preference?

drvinceknight · 2017-01-17T09:51:00Z

docs/tutorials/contributing/strategy/writing_test_for_the_new_strategy.rst

+  in the four possible second rounds of play, depending on the move the the
+  strategy and the opponent in the first round.
+* `responses_test(responses, history1=None, history2=None, ...)` is a powerful
+  test that can handle a variety of situations, testing the first X actions, the


n actions? (not overly insistant but personally prefer n for discrete variables...)

drvinceknight · 2017-01-17T09:55:40Z

docs/tutorials/contributing/strategy/writing_test_for_the_new_strategy.rst

 :code:`strategy` method in the :code:`TitForTat` class works as expected:

 1. If the opponent's last strategy was :code:`C`: then :code:`TitForTat` should
   cooperate::

-    self.responses_test([C] * 4, [C, C, C, C], [C])
+    self.responses_test([C], [C] * 4, [C, C, C, C])


This is still not immediately clear to me, perhaps for clarity write this (and the class above) with:

self.responses_test(responses=[C], history1=[C, C, C, C], history2=[C, C, C, C]),

I also wonder if the variable names (here and in the class) should be opponent_history, player_history?

In your explanation here you have not made it clear why you have a list of 4 C, perhaps a sentence like here we also are able to test the response :code:C is obtained after a history of 4 consecutive C for each player. (I also feel that the mix of [C] *4 and [C, C, C, C] is not helpful: perhaps we just stick with one of them?).

I just used the test that was already there; I'll improve the language.

drvinceknight · 2017-01-17T10:23:04Z

docs/tutorials/contributing/strategy/writing_test_for_the_new_strategy.rst

-        self.assertEqual(P1.strategy(P2), 'C')
+   This is equivalent to choosing an opponent will play C or D as needed and
+   checking the next move. This function can also take an optional random seed
+   argument `seed`.

 3. The member function :code:`responses_test` takes arbitrary histories for each


If I'm not mistaken you have already mentioned responses_test on line 77. Could this be brought together?

drvinceknight · 2017-01-17T10:24:02Z

docs/tutorials/contributing/strategy/writing_test_for_the_new_strategy.rst


 2. If the opponent's last strategy was :code:`D`: then :code:`TitForTat` should
   defect::

-    self.responses_test([C] * 5, [C, C, C, C, D], [D])
+    self.responses_test([D], [C] * 5, [C, C, C, C, D])

 We have added some convenience member functions to the :code:`TestPlayer` class.


This has now been said above right?

drvinceknight · 2017-01-17T10:24:54Z

docs/tutorials/contributing/strategy/writing_test_for_the_new_strategy.rst

+   and so will not actually test the strategy correctly. The test suite will
+   warn you if it detects a mismatch in simulated history and actual history.
+
+   Note also that in general it is not a good idea to manually set the history


Is this sentence necessary?

About setting the history? Indeed it is, setting the history directly will often not set the member instances correctly leading to an erroneous test.

Yes I agree with what you say in the sentence (because I know why which the reader might not) but with this framework there's not much need for the sentence (as people are unlikely to do it right?) or at least you could add the explanation?

drvinceknight · 2017-01-17T10:26:18Z

docs/tutorials/getting_started/contributing.old

@@ -113,7 +113,7 @@ opponent has not played yet) will cooperate::
        except IndexError:


This file should be deleted.

drvinceknight · 2017-01-17T10:30:05Z

docs/tutorials/contributing/strategy/writing_test_for_the_new_strategy.rst

+        self.first_play_test(C, seed=11)
+        self.first_play_test(D, seed=23)
+
+* `second_play_test(rCC, rCD, rDC, rDD, seed=None)` tests the strategies actions


You discuss these 3 functions lower down in terms of the member functions, I think you could remove this paragraph and just start by illustrating with an example (TitForTat). Or perhaps something else but it's not ideal to have these twice.

Maybe the extra examples are useful?

Yes the extra example (that was not the point I was making). You introduce these functions twice, from a newcomer/beginner's point of view that can be confusing.

I'll write a modification and send you a PR and you can see what I'm trying to say.

I'll also fix your rest syntax (you can build the docs locally by running make html).

drvinceknight · 2017-01-17T15:16:30Z

Any thoughts on:

I also wonder if the variable names (here and in the class) should be opponent_history, player_history?

drvinceknight · 2017-01-17T15:46:56Z

Have opened #825 to this branch with what I had in mind.

Also: fix rst syntax.

marcharper · 2017-01-18T04:07:12Z

I'm fine with changing the argument names for response test. Sorry I had to get on a plane and didn't get to finish and push the updates.

drvinceknight · 2017-01-18T04:08:42Z

I'm fine with changing the argument names for response test. Sorry I had to get on a plane and didn't get to finish and push the updates.

No problem at all 👍 :)

drvinceknight · 2017-01-18T04:27:29Z

I'm fine with changing the argument names for response test.

Let me know if you'd like me to do this or we could have this as a further PR :)

marcharper · 2017-01-18T04:28:36Z

I can do it -- pycharm can globally find-replace

drvinceknight · 2017-01-18T04:29:45Z

I can do it -- pycharm can globally find-replace

Winning :)

marcharper · 2017-01-18T05:14:36Z

@drvinceknight Let me know if I've missed anything, and if you'd like docstrings for test_strategy.

drvinceknight · 2017-01-18T06:19:21Z

@drvinceknight Let me know if I've missed anything, and if you'd like docstrings for test_strategy

No: you got everything for me. Thanks! This is an awesome contribution and I think really tidies up our testing framework! :D

You may note some additional warnings now in the test suite -- if the requested history of play is impossible then a warning will be raised (but the test can still pass). This is to encourage the writing of better tests. We can suppress these.

Yeah: we can open up an issue to clean up the warnings we're now getting once this is in. 👍

marcharper force-pushed the testing branch from 34572f2 to 98725d0 Compare January 16, 2017 07:00

marcharper added 4 commits January 16, 2017 09:11

Rework basic tests and update a number of explicit tests

e8a96db

Update MockPlayer and some PEP8 / docstring fixes

6be7c6a

Update docs, rework simulate play and some test internals, add warnin…

cb61be5

…gs for mismatched histories

Rewrite TestHeadsUp with Match class and rename

0865652

marcharper force-pushed the testing branch from fbd899a to 0865652 Compare January 16, 2017 17:16

Mostly PEP8 modifications

33f806f

marcharper force-pushed the testing branch from a33ff3d to 43493c9 Compare January 16, 2017 20:11

More test improvements and PEP8

5b474b2

marcharper force-pushed the testing branch from 6408ff7 to 5b474b2 Compare January 16, 2017 20:32

marcharper added the ready-for-review label Jan 16, 2017

Fix rst syntax.

81853d6

drvinceknight requested changes Jan 17, 2017

View reviewed changes

marcharper added 2 commits January 17, 2017 07:05

Modify the test writing doc

2e0a5a4

remove contributing.old doc

1176f64

drvinceknight mentioned this pull request Jan 17, 2017

Re structure testing docs. #825

Merged

Re structure testing docs.

67a86eb

Also: fix rst syntax.

Change argument names for test_reponses

a6b0087

drvinceknight approved these changes Jan 18, 2017

View reviewed changes

drvinceknight added ready-to-merge and removed ready-for-review labels Jan 18, 2017

meatballs merged commit 22e0357 into master Jan 18, 2017

meatballs deleted the testing branch January 18, 2017 15:11

drvinceknight mentioned this pull request Jan 19, 2017

Need to improve documentation for strategy test functions #726

Closed



		def simulate_play(P1, P2, h1=None, h2=None):
		def simulate_play(player1, player2, h1=None, h2=None):

		@@ -113,7 +113,7 @@ opponent has not played yet) will cooperate::
		except IndexError:

Rework basic tests and update a number of explicit tests #820

Rework basic tests and update a number of explicit tests #820

Conversation

marcharper commented Jan 16, 2017 • edited Loading

marcharper commented Jan 16, 2017

drvinceknight commented Jan 16, 2017

drvinceknight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drvinceknight commented Jan 17, 2017

drvinceknight commented Jan 17, 2017

marcharper commented Jan 18, 2017

drvinceknight commented Jan 18, 2017

drvinceknight commented Jan 18, 2017

marcharper commented Jan 18, 2017

drvinceknight commented Jan 18, 2017

marcharper commented Jan 18, 2017

drvinceknight commented Jan 18, 2017

marcharper commented Jan 16, 2017 •

edited

Loading