-
Notifications
You must be signed in to change notification settings - Fork 264
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
Adding a new strategy gotchas #1370
Comments
Gosh sorry I've just seen your comment on #1364, my bad for missing it.
I think I'm in favour of that approach. It's not ideal but is probably the most sensible on balance. |
Removes some tests that do not target specific strategy behaviors and can change when new strategies are added.
There are already some tests in |
* [#1370] Remove some tests from test_meta.py Removes some tests that do not target specific strategy behaviors and can change when new strategies are added. * Add a property based test for valid strategies Not sure if this is overkill @marcharper, just thought it could be worth having. * Run isort. Co-authored-by: Vince Knight <vincent.knight@gmail.com>
In PR #1364, adding a new strategy tripped some of the tests for the meta strategies because it induces a change in the behavior of those strategies. This is not ideal since it adds complexity to the process of adding new strategies.
There are various ways we could mitigate this:
At the least we should update the documentation. There are also documentation tests that tend to fail when adding a new strategy (counts of how many strategies there are, for example), but those seem to serve a somewhat useful purpose.
The text was updated successfully, but these errors were encountered: