Skip to content

Document passing a match length attribute to responses_test #874

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
drvinceknight opened this issue Mar 3, 2017 · 10 comments
Closed

Document passing a match length attribute to responses_test #874

drvinceknight opened this issue Mar 3, 2017 · 10 comments

Comments

@drvinceknight
Copy link
Member

This is currently not documented at http://axelrod.readthedocs.io/en/latest/tutorials/contributing/strategy/writing_test_for_the_new_strategy.html

but it is implemented: https://github.com/Axelrod-Python/Axelrod/blob/master/axelrod/tests/unit/test_backstabber.py#L36 👍

@marcharper
Copy link
Member

marcharper commented Mar 4, 2017

I think we should try to move to testing direct matches whenever possible rather than enforcing potentially impossible histories. In this case we could test that the last action is a defection by simply running a short match and checking the last action.

Mock histories are ok in some cases, especially when finding just the right random seed would be very difficult, but as all the build warnings show now we're overusing this trick.

@drvinceknight
Copy link
Member Author

That sounds good to me. Would involve going through all the tests and structuring the docs a bit differently (just reordering what's there).

@drvinceknight
Copy link
Member Author

I was thinking of going through and sorting out those warnings at some point so I can change over to matches too. It'll probably be a slow burn but if no one else wants to do it I'm happy to assign this to myself...

@drvinceknight
Copy link
Member Author

Although thinking about it, we should do this carefully and from a review point of view it's probably easier to do it small set of strategies at a time. Perhaps I can do a few and then we can open a nice descriptive issue.

@drvinceknight
Copy link
Member Author

drvinceknight commented Mar 4, 2017

@marcharper I've just taken a quick look but the docs for TestMatch.versus_test are incorrect. http://axelrod.readthedocs.io/en/latest/tutorials/contributing/strategy/writing_test_for_the_new_strategy.html currently reads:

class TestTF2TvsBully(TestMatch):
    """Test Tit for Two Tats vs Bully"""
    def test_rounds(self):
        outcomes = [[C, D], [C, D], [D, D], [D, C], [C, C], [C, D], [C, D], [D, D]]
        self.versus_test(axelrod.TitFor2Tats, axelrod.Bully, outcomes)

However the versus_test method of that class is:

>>370     def versus_test(self, player1, player2, expected_actions1,                                                                                  
  371                     expected_actions2, noise=None, seed=None):                                                                                  
  372         """Tests a sequence of outcomes for two given players."""                                                                               
  373         if len(expected_actions1) != len(expected_actions2):                                                                                    
  374             raise ValueError("Mismatched History lengths.")                                                                                     
  375         if seed:                                                                                                                                
  376             axelrod.seed(seed)                                                                                                                  
  377         turns = len(expected_actions1)                                                                                                          
  378         match = axelrod.Match((player1, player2), turns=turns, noise=noise)                                                                     
  379         match.play()                                                                                                                            
  380         # Test expected sequence of play.                                                                                                       
  381         for i, (outcome1, outcome2) in enumerate(                                                                                               
  382             zip(expected_actions1, expected_actions2)):                                                                                         
  383             player1.play(player2)                                                                                                               
  384             self.assertEqual(player1.history[i], outcome1)                                                                                      
  385             self.assertEqual(player2.history[i], outcome2)

(the arguments don't match up)

We could however just remove the TestMatch class and in the documentation show that we recommend testing by creating matches between two players? This improves the verbosity of the tests in my opinion (and makes it clearer to newcomers).

So for example here I have rewritten the test for TitForTat: 3933658

Let me know what you think.

@marcharper
Copy link
Member

I think this is on the right track. We should think about combining our responses test and the heads up test using our Match class (which didn't exist way back when). responses_test is actually quite powerful since it can check the player's attributes at steps along the way and we can't do that with matches. So while I like this approach it's not going to cover some of the test cases.

@drvinceknight
Copy link
Member Author

I think this is on the right track. We should think about combining our responses test and the heads up test using our Match class (which didn't exist way back when). responses_test is actually quite powerful since it can check the player's attributes at steps along the way and we can't do that with matches. So while I like this approach it's not going to cover some of the test cases.

Very good point. I think I see a good way of doing this. I'm going to give something a go later today:

  • Implement a versus_test in the TestPlayer class which would take the best of responses_test along with the Match class.
  • Essentially depreciate the responses_test (although leave it in place until we have refactored all tests).
  • Refactor some tests (TitForTat and maybe a couple of others)
  • Update the docs :)

@drvinceknight
Copy link
Member Author

I've opened #875 with my suggestion :) 👍

@marcharper
Copy link
Member

Thanks. This is looking like a better approach -- easier to understand and it dogfoods our Match class, which is always a good thing.

@drvinceknight
Copy link
Member Author

Closed by #875

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants