-
Notifications
You must be signed in to change notification settings - Fork 263
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
Add missing tests for filters.py #899
Conversation
axelrod/tests/unit/test_filters.py
Outdated
classifier = { | ||
'stochastic': True, | ||
'inspects_source': False, | ||
'memory_depth': 'infinity', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
float('inf')
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how the test passes as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scenario where a strategy has memory_depth': 'infinity'
is precisely what this test is for. There's a line in the filter that catches it and converts it to a float and it's that line that comes up on the coverage report as not covered.
I think I had to add the line to cope with one or two strategies that have this entry and were causing the API to fall over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I had to add the line to cope with one or two strategies that have this entry and were causing the API to fall over.
I don't think we have any strategies with that entry (or at least we shouldn't):
>>> import axelrod as axl
>>> players = [s() for s in axl.strategies]
>>> mem_lengths = [p.classifier['memory_depth'] for p in players]
>>> set(mem_lengths)
{0, 1, 2, 3, 4, 5, 6, 9, 10, 11, 12, 16, 20, 40, 200, inf}
Did you pergaps put this in so that the API could pass infinity
to a filter? (Should that be done at the API level perhaps? So that the API converts infinity
to a float
?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possibly! I'll swap the test around
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope. looking at the code, it's definitely there to cope with strategies having 'infinity' but I can't see any that do. Perhaps there were at some point.
We can either get rid of the line or add a test for it. Which do we prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can either get rid of the line or add a test for it. Which do we prefer?
I'd vote for getting rid of the line.
Looks good to me (subject to travis). I'm applying the |
@meatballs Is "infinity" there for API purposes? If not then it makes sense to do so, since float('inf') isn't a thing in javascript / JSON. |
Could we let the API convert |
Yes, I suppose it could. |
No description provided.