keeping the docs honest #99
Conversation
tox.ini
Outdated
| @@ -28,7 +28,7 @@ commands = | |||
| {envpython} tests/manage.py behave --tags=~@failing --format=progress | |||
|
|
|||
| [testenv:bandit] | |||
| deps = bandit | |||
| deps = bandit==1.5.1 | |||
There was a problem hiding this comment.
Please, make this <1.6 instead.
Though the related problem should be fixed. Strange.
There was a problem hiding this comment.
Can do.
I think the linked issue is performance related?
It looks like the failing job https://travis-ci.org/behave/behave-django/jobs/551031584 failed to honor the tox configuration here https://github.com/behave/behave-django/blob/master/tox.ini#L66. But that's probably a separate issue to the core issue being raised in this PR.
| @@ -120,7 +120,7 @@ def run_hook(self, name, context, *args): | |||
| django_test_runner.setup_testclass(context) | |||
| django_test_runner.setup_fixtures(context) | |||
| django_test_runner.setup_test(context) | |||
| behave_run_hook(self, 'django_ready', context) | |||
| behave_run_hook(self, 'django_ready', context, *args) | |||
There was a problem hiding this comment.
That looks like a bug fix. Could you try to reproduce the bug with a unit test? That would be helpful.
There was a problem hiding this comment.
@bittner I'm not sure how I'd unit test this without piggybacking off of the cli tests. I.e. I could mock the django_hook function in a test that looks like https://github.com/behave/behave-django/blob/master/tests/unit/test_cli.py#L68-L73. Then assert the mock was called with a context and a scenario instance...
However it looks like this is just ridding on the core environment functionality where the runner calls the appropriate hook within a behave model. https://github.com/behave/behave/blob/master/behave/model.py#L968 ... So I'm pretty sure the only arg at this stage of the patch is a Scenario instance.
| @@ -24,5 +24,5 @@ def before_scenario(context, scenario): | |||
| context.multi_db = True | |||
|
|
|||
|
|
|||
| def django_ready(context): | |||
| def django_ready(context, scenario): | |||
There was a problem hiding this comment.
If possible the additional parameter should be optional.
Maybe change the documentation instead, or in addition?
There was a problem hiding this comment.
This is a strange situation to be in for sure. Either leave the functionality as is and change the docs, which would be a shame in my opinion. Given that there may be useful information in the scenario where you want to create certain data states across a number of features given a scenario pattern.
Whereas if you add this PR as is it'll break existing projects raising IndexError. I'm open to suggestions on how to make it optional. But like I said, I'm just looking into adding this package into a project. So I'm fairly new to the behave source.
Perhaps a new cli flag could do the trick where the default remains. Not sure though, it's up to you all.
There was a problem hiding this comment.
Maybe change this to be something like monkey_patch_behave(django_test_runner, options['django_hook_add_scenario'])
There was a problem hiding this comment.
This will always be None though wouldn't it? By passing *args you're not really solving anything, why not just leave it as is, and change the docs to say def django_ready(context): ?
There was a problem hiding this comment.
@anthonyalmarza I think @kingbuzzman is correct. Would you mind to just fix the docs that way?
Hi, I was just trying this out on a project and found that when I followed the docs I got a
TypeError: django_ready() missing 1 required positional argument: 'scenario'... Hope it helps.It also looks as though there's been a minor release to
banditthat's causing the existing tests to fail on new builds. So I've just locked the version to that of the last successful build.