Add a versus_test method to TestPlayer #875

drvinceknight · 2017-03-05T15:03:51Z

Addresses #874 with a tweaked testing framework.

The versus_test method removes the option to set player histories and can be used to test strategies (and player attributes during matches) by creating a match between the player in question and an opponent.

The opponent can either be an actual player from the library or is defined as a cycle by a passed sequence of actions (there are examples of this in the docs and test_titfortat.py).

I've refactored all the tests in test_titfortat.py (which is now one module that raises no warnings).

When/if we're happy with this as a suggestion, we should open an issue about refactoring all the tests to use this approach. (This would be a big/important piece of work so we should ask for small contributions at a time to make sure we don't miss anything with the reviews).

Once that refactor is complete we could potentially remove unused methods in the TestPlayer class.

marcharper · 2017-03-06T05:03:45Z

I like most of this. Some quick thoughts:

Having opponent be a player or a sequence is a bit confusing -- using a MockPlayer would be better IMO, that's its purpose.
We might be able to get away with only having init_kwargs, or it may be better to tackle that issue before we rewrite a bunch of tests.

drvinceknight · 2017-03-06T07:57:37Z

Having opponent be a player or a sequence is a bit confusing -- using a MockPlayer would be better IMO, that's its purpose.

Yeah I wasn't completely sure about having two input possibilities. I'll make this tweak.

We might be able to get away with only having init_kwargs, or it may be better to tackle that issue before we rewrite a bunch of tests.

Can you expand on this a bit more? I'm not too sure I follow which issue you mean.

drvinceknight · 2017-03-06T08:16:03Z

I've also just pushed a refactor of the Random strategy. I've used init_kwargs there.

marcharper · 2017-03-06T08:23:11Z

Issue #706 -- maybe we could make all the Player args into kwargs, which I think would help with #706 as well.

drvinceknight · 2017-03-06T08:27:38Z

Issue #706 -- maybe we could make all the Player args into kwargs, which I think would help with #706 as well.

Sure thing, I'll remove the init_args option for now so that it will push us in that direction.

marcharper · 2017-03-06T08:29:01Z

It's ok to use test_reset.

drvinceknight · 2017-03-06T08:31:53Z

It's ok to use test_reset.

Did you mean to type that here or in the other issue? Surely that's overwriting the test_reset method of the parent class?

marcharper · 2017-03-07T06:55:40Z

axelrod/tests/unit/test_grumpy.py

@@ -45,7 +45,7 @@ def test_strategy(self):
                            init_kwargs={"grumpy_threshold": 3,
                                         "nice_threshold": 0})

-    def test_reset(self):
+    def test_reset_state(self):


I think this test is unnecessary -- the default test catches any attribute changes.

marcharper · 2017-03-07T06:57:04Z

axelrod/tests/unit/test_player.py

+            actions is passed, a Mock Player is created that cycles over that
+            sequence.
+        expected_outcomes: List
+            the expected outcomes of the match (list of tuples of actions).


marcharper · 2017-03-07T06:57:12Z

axelrod/tests/unit/test_player.py

+            `{length:-1}` implies that the players do not know the length of the
+            match.
+        attrs: dict
+            dictionary of internal attributes to check at the end of all plays


marcharper · 2017-03-07T06:59:23Z

axelrod/tests/unit/test_rand.py

-        self.responses_test([C], [C, D, C], [C, C, D], seed=1)
+
+        opponent = axelrod.MockPlayer()
+        outcomes = [(C, C), (D, C), (D, C)]


This tests the histories above but not the next move, so it's not exactly equivalent. Doesn't matter in this case of course.

marcharper · 2017-03-07T07:00:08Z

axelrod/tests/unit/test_titfortat.py

+        self.second_play_test(rCC=C, rCD=D, rDC=C, rDD=D)
+
+        # Play against opponents
+        outcomes = [(C, C), (C, D), (D, C), (C, D)]


Same comment as before re: not checking the last move anymore.

drvinceknight · 2017-03-07T14:02:05Z

@marcharper the particular test in test_grumpy was not covered by the default case as it's checking a non default value but your comment got me to find a bug in our default test case. It was in effect not testing anything (it was simply comparing the attributes of the clone to the attributes of the clone and not the reset player).

I've fixed this (cff2e53 and a refactor here: b7ce33a) and ended up having to fix some strategies that were then failing.

This is worth a slow and thorough review in case I've missed anything so no rush :)

marcharper · 2017-03-07T15:59:13Z

axelrod/tests/unit/test_player.py

-        for k, v in clone.__dict__.items():
-            if isinstance(v, np.ndarray):
-                self.assertTrue(np.array_equal(v, getattr(clone, k)))
+        for attribute, value in player.__dict__.items():


Would player == clone work here now?

marcharper · 2017-03-07T16:02:18Z

axelrod/tests/unit/test_meta.py

+        player.play(opponent)
+        player.reset()
+        for i, p in enumerate(player.team):
+            self.assertEqual(len(p.history), 0)


Can we use player == clone here?

marcharper · 2017-03-07T16:03:19Z

axelrod/tests/unit/test_player.py

+            axelrod.seed(seed)
+            player.play(opponent)
+
+        player.reset()


player == clone?

marcharper · 2017-03-07T16:07:23Z

That's interesting. I remember catching a subtle bug with MetaPlayer with that test so that's surprising that it worked at all! I like the new additions, looks like maybe we could remove some redundant code by using player == clone in a few places.

We might consider updating MetaPlayer's clone method to clone all the team members, or just take a closer look at it.

drvinceknight · 2017-03-07T16:36:05Z

That's interesting. I remember catching a subtle bug with MetaPlayer with that test so that's surprising that it worked at all! I like the new additions, looks like maybe we could remove some redundant code by using player == clone in a few places.

You might be able to tell from the commit history that I implemented a player __eq__ but felt that that was stepping a bit too far for this PR so I reverted it (we should squash these commits for with the merge). Without the __eq__ method player == clone would just look at instance equality (so be equivalent to player is clone). That could be added at another date.

We might consider updating MetaPlayer's clone method to clone all the team members, or just take a closer look at it.

Perhaps for another PR?

This does it by specifically creating the Matches.

- Refactor test for Tf2Ts. - Refactor tests for 2TfT - Refactor tests for Bully. - Refactor tests for SneakyTitForTat. - Refactor test for suspicious TfT. - Refactor test for AntiTitForTat - Refactor tests for HardTfT - Refactor tests for HardTf2T - Refactor OmegaTfT tests. - Refactor tests for Gradual. - Refactor tests for SlowTitForTwoTats - Refactor tests for AdaptiveTfT - Refactor tests for SpitefulTfT Also implement correct check for seed is not None. If we check `if seed` then when seed is passed as 0 it will not be set.

Add a state variable to init.

Add equality for a FSM.

This reverts commit 14e6a6b.

This makes it easier to test specific new attributes that might not fall under the scope of the global case.

drvinceknight · 2017-03-07T16:50:32Z

I've just rebased on to master and caught a misclassification of stalker :)

I'm not sure if this means I'm going to miss any comments you wrote but I think all of them were suggesting player == clone which (unless I'm missing something) won't work because of the fact that we haven't got a __eq__ method implemented but that could be another issue to raise/discuss?

marcharper · 2017-03-08T03:39:57Z

OK. __eq__ was still there when I was commenting. I would vote to add it back in, but it can be another PR.

marcharper · 2017-03-08T03:43:20Z

axelrod/strategies/finite_state_machines.py

@@ -57,6 +64,7 @@ def __init__(self, transitions=None, initial_state=None,
            initial_action = C
        super().__init__()
        self.initial_state = initial_state
+        self.state = initial_state


What's the purpose of this line?

Needed for the reset test. The cloned player did not have a self.state attribute if it had not played yet.

marcharper · 2017-03-08T03:47:05Z

Do we want to change expected_outcomes to expected_actions or something else more inline with the terminology elsewhere in the library?

drvinceknight · 2017-03-08T07:20:59Z

OK. eq was still there when I was commenting. I would vote to add it back in, but it can be another PR.

Cool. Once this is in I'll open a couple of issues:

To refactor the tests;
To add an __eq__ method in to the players and refactor some of the base tests;
Once 1. is complete to refactor the base tests to remove anything that is not needed anymore.

Do we want to change expected_outcomes to expected_actions or something else more inline with the terminology elsewhere in the library?

Good call. I've pushed that.

drvinceknight · 2017-03-09T07:46:46Z

@marcharper in bc398f2 I've gone a step further and removed first_play_test and second_play_test from the docs and test_tit_for_tat, test_rand. (I also added a couple more versus_tests in test_tit_for_tat to make sure all states were being tested.)

My thinking is that we can test those properties using matches and it moves us away from the potential history mistmatches that can still occur with second_play_test.

Can always revert if we don't think it's a good idea.

marcharper · 2017-03-09T07:57:24Z

I'd prefer to keep those tests -- the warnings for those are suppressed already. Or -- let's take that discussion to a new issue / PR?

This reverts commit bc398f2.

drvinceknight · 2017-03-09T07:58:47Z

I'd prefer to keep those tests -- the warnings for those are suppressed already. Or -- let's take that discussion to a new issue / PR?

Fine by me, I've reverted the commit.

meatballs · 2017-03-09T11:31:35Z

This one looks ready to roll to me. Anyone object?

drvinceknight · 2017-03-09T11:48:06Z

This one looks ready to roll to me. Anyone object?

Not from me :)

drvinceknight mentioned this pull request Mar 5, 2017

Document passing a match length attribute to responses_test #874

Closed

marcharper reviewed Mar 7, 2017

View reviewed changes

axelrod/tests/unit/test_player.py

axelrod.seed(seed)

player.play(opponent)

player.reset()

Copy link

Member

marcharper Mar 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

player == clone?

drvinceknight added 11 commits March 7, 2017 16:38

Rewrite test for TitForTat.

84f44f7

This does it by specifically creating the Matches.

Write a versus_test method in TestPlayer.

5deddbc

Make Mock player cycle.

b22c0a2

Reword the documentation.

48ec039

Add variable names to method calls.

8070070

Remove option to versus test with a sequence.

ac4164f

Refactor tests for Random.

4747071

Rename test resets.

c29820f

Remove option to use init_args.

0d9ba49

Address comments about tests.

8746199

drvinceknight added 10 commits March 7, 2017 16:38

Fix hmm.

dcb21d5

Add a state variable to init.

Fix fsm player.

eeb58f3

Add equality for a FSM.

Correct test for hmm and fsm eq.

aae3665

Fix type check in reset test.

87e7781

Add specific test for meta players.

db5637e

Rename extra test for grumpy.

72fb241

Revert "Fix test for calculator + implemented player eq"

2f342b9

This reverts commit 14e6a6b.

Refactor out a sub method in test player.

1a6eacc

This makes it easier to test specific new attributes that might not fall under the scope of the global case.

Correct failing test.

83d05dc

Correct classification of Stalker.

e2ccc13

drvinceknight force-pushed the 874 branch from 6b9da7a to e2ccc13 Compare March 7, 2017 16:47

marcharper previously approved these changes Mar 8, 2017

View reviewed changes

marcharper reviewed Mar 8, 2017

View reviewed changes

Substitute: expected_outcomes -> expected_actions

988d288

marcharper approved these changes Mar 8, 2017

View reviewed changes

Remove versus_test + 1st_play intft, rand and docs

bc398f2

Revert "Remove versus_test + 1st_play intft, rand and docs"

3d81683

This reverts commit bc398f2.

meatballs merged commit ab01b8c into master Mar 9, 2017

meatballs deleted the 874 branch March 9, 2017 11:54

drvinceknight mentioned this pull request Mar 9, 2017

Refactor all strategy tests #884

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a versus_test method to TestPlayer #875

Add a versus_test method to TestPlayer #875

drvinceknight commented Mar 5, 2017

marcharper commented Mar 6, 2017

drvinceknight commented Mar 6, 2017

drvinceknight commented Mar 6, 2017

marcharper commented Mar 6, 2017

drvinceknight commented Mar 6, 2017

marcharper commented Mar 6, 2017

drvinceknight commented Mar 6, 2017

marcharper Mar 7, 2017 •

edited

Loading

marcharper Mar 7, 2017

marcharper Mar 7, 2017

marcharper Mar 7, 2017

marcharper Mar 7, 2017

drvinceknight commented Mar 7, 2017

marcharper Mar 7, 2017

marcharper Mar 7, 2017

marcharper Mar 7, 2017

marcharper commented Mar 7, 2017

drvinceknight commented Mar 7, 2017

drvinceknight commented Mar 7, 2017

marcharper commented Mar 8, 2017

marcharper Mar 8, 2017

drvinceknight Mar 8, 2017

marcharper commented Mar 8, 2017

drvinceknight commented Mar 8, 2017

drvinceknight commented Mar 9, 2017

marcharper commented Mar 9, 2017

drvinceknight commented Mar 9, 2017

meatballs commented Mar 9, 2017

drvinceknight commented Mar 9, 2017

Add a versus_test method to TestPlayer #875

Add a versus_test method to TestPlayer #875

Conversation

drvinceknight commented Mar 5, 2017

marcharper commented Mar 6, 2017

drvinceknight commented Mar 6, 2017

drvinceknight commented Mar 6, 2017

marcharper commented Mar 6, 2017

drvinceknight commented Mar 6, 2017

marcharper commented Mar 6, 2017

drvinceknight commented Mar 6, 2017

marcharper Mar 7, 2017 • edited Loading

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 Mar 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcharper commented Mar 7, 2017

drvinceknight commented Mar 7, 2017

drvinceknight commented Mar 7, 2017

marcharper commented Mar 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcharper commented Mar 8, 2017

drvinceknight commented Mar 8, 2017

drvinceknight commented Mar 9, 2017

marcharper commented Mar 9, 2017

drvinceknight commented Mar 9, 2017

meatballs commented Mar 9, 2017

drvinceknight commented Mar 9, 2017

marcharper Mar 7, 2017 •

edited

Loading