Skip to content
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.

Sign up for GitHub

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

keeping the docs honest #99

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

keeping the docs honest #99

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a3491ed
keeping the docs honest
anthonyalmarza Jun 26, 2019
7f2e785
lock down bandit version
anthonyalmarza Jun 26, 2019
3e5c24b
Update tox.ini
anthonyalmarza Jun 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion behave_django/environment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

That looks like a bug fix. Could you try to reproduce the bug with a unit test? That would be helpful.

Copy link
Member

Choose a reason for hiding this comment

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

Will that fill in the scenario correctly?

@jenisys @mixxorz

Copy link
Author

Choose a reason for hiding this comment

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

@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.


if name == 'after_scenario':
django_test_runner.teardown_test(context)
Expand Down
2 changes: 1 addition & 1 deletion tests/acceptance/environment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,5 @@ def before_scenario(context, scenario):
context.multi_db = True


def django_ready(context):
def django_ready(context, scenario):
Copy link
Member

Choose a reason for hiding this comment

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

If possible the additional parameter should be optional.

Maybe change the documentation instead, or in addition?

Copy link
Author

@anthonyalmarza anthonyalmarza Jun 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe change this to be something like monkey_patch_behave(django_test_runner, options['django_hook_add_scenario'])

Copy link
Contributor

Choose a reason for hiding this comment

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

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): ?

Copy link
Member

Choose a reason for hiding this comment

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

@anthonyalmarza I think @kingbuzzman is correct. Would you mind to just fix the docs that way?

context.django = True
2 changes: 1 addition & 1 deletion tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ commands =
{envpython} tests/manage.py behave --tags=~@failing --format=progress

[testenv:bandit]
deps = bandit
deps = bandit==1.5.1
Copy link
Member

Choose a reason for hiding this comment

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

Please, make this <1.6 instead.

Though the related problem should be fixed. Strange.

Copy link
Author

Choose a reason for hiding this comment

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

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.

commands = bandit -r --ini tox.ini

[testenv:clean]
Expand Down