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.

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

Adds the ability to use --behave-runner in the behave command #100

Closed
wants to merge 27 commits into from
Closed

Adds the ability to use --behave-runner in the behave command #100

wants to merge 27 commits into from

Conversation

kingbuzzman
Copy link
Contributor

@kingbuzzman kingbuzzman commented Sep 6, 2019

I'm making a rather large test migration from Aloe to Behave.

$ find . -name *.feature | xargs cat | wc -l
  157533

Currently, all our tests are inside our apps like so: (py + feature files all in the same folder inside EACH app)

<project>/<app>/features
                        /__init__.py
                        /sometest.feature
                        /sometest_steps.py
                        /steps.py

I do not want to make this migration any more painful than it needs to be, so my plan is to modify behave.runner.Runner BehaviorDrivenTestRunner to find my tests in the appropriate place. Right now there is no way for me to modify the way this works. This is the exact reason django exposes --testrunner and i was very surprised this was something overlooked in this project.

Making this modification works great: ./manage.py behave --behave-runner myproject.runner.Runner

import os

from behave.runner import Runner as BaseRunner, load_step_modules
from django.conf import settings


class Runner(BaseRunner):

    def load_step_definitions(self, extra_step_paths=None):
        step_paths = [] + list(extra_step_paths or [])
        for root, dirs, _ in os.walk(settings.BASE_DIR):
            for dir in dirs:
                if dir.endswith('features'):
                    full_path = os.path.join(root, dir)
                    step_paths.append(full_path)

        load_step_modules(step_paths)

tox.ini Outdated Show resolved Hide resolved
Copy link
Member

@bittner bittner left a comment

Choose a reason for hiding this comment

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

This is a rather large PR.

Can you please provide an initial description, which provides the motivation for all your changes and/or a reference to an open issue (in this repository)?

Specifically, I have a hard time to see why you prefer to replace a short command line option by a rather lengthy, hard to memorize one.

docs/usage.rst Outdated Show resolved Hide resolved
behave_django/management/commands/behave.py Outdated Show resolved Hide resolved
behave_django/management/commands/behave.py Outdated Show resolved Hide resolved
behave_django/management/commands/behave.py Outdated Show resolved Hide resolved
behave_django/management/commands/behave.py Outdated Show resolved Hide resolved
behave_django/management/commands/behave.py Outdated Show resolved Hide resolved
behave_django/management/commands/behave.py Outdated Show resolved Hide resolved
docs/usage.rst Outdated Show resolved Hide resolved
docs/usage.rst Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@kingbuzzman kingbuzzman changed the title Adds the ability to use --testrunner in the behave command Adds the ability to use --behave-runner in the behave command Sep 18, 2019
@kingbuzzman
Copy link
Contributor Author

@bittner I drastically reduced the changes, turns out i was wrong with my original approach since the TestCase is not used to run the tests, I discovered this when i finally implemented the code. Let me know if you want any additional changes.

@kingbuzzman
Copy link
Contributor Author

@bittner friendly reminder, would love some feedback.

@kingbuzzman
Copy link
Contributor Author

@bittner ?

Copy link
Member

@bittner bittner left a comment

Choose a reason for hiding this comment

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

Hmm, not bad! I love how much you reduced the code needed for this contribution!

Looks like there is some more potential to reduce the code. Go for it! 💯

behave_django/environment.py Outdated Show resolved Hide resolved
behave_django/management/commands/behave.py Outdated Show resolved Hide resolved
behave_django/management/commands/behave.py Outdated Show resolved Hide resolved
docs/usage.rst Outdated Show resolved Hide resolved
behave_django/management/commands/behave.py Outdated Show resolved Hide resolved
@kingbuzzman
Copy link
Contributor Author

@bittner Made changes, let me know.

@kingbuzzman
Copy link
Contributor Author

@bittner ?

@bittner
Copy link
Member

bittner commented Nov 20, 2019

Sorry about that. Taking a look now.

Copy link
Member

@bittner bittner left a comment

Choose a reason for hiding this comment

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

Sorry, still not all good yet. 😟


from behave import __main__ as main
Copy link
Member

Choose a reason for hiding this comment

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

I think you want to import main like in the management command:

from behave.__main__ import main as behave_main

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cant do this, need to be able to override the function inside behave.__main__ called run_behave

@@ -1,7 +1,9 @@
from copy import copy
from functools import partial
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why it's important to use partial? What's your motivation for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

behave_main.run_behave() takes an optional parameter called runner_class, if this is not specified, it defaults to behave.runner.Runner further down the code.

https://github.com/behave/behave/blob/9a438ebb29a21f3953009381e6949e8f64f57b07/behave/__main__.py#L52

With partial all im doing is keeping the same callable with no arguments as before but setting the "default" value of runner_class to what ever class may be inside behave_runner

docs/usage.rst Outdated Show resolved Hide resolved
behave_django/management/commands/behave.py Outdated Show resolved Hide resolved
Copy link
Member

@bittner bittner left a comment

Choose a reason for hiding this comment

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

Just a few more things to change until we reach the finish line. Congratulations! 👍

Would you mind squashing all the commits into one, two or three commits only? Whatever makes sense to put in single commits together. That would be great. The version bump, probably, as a separate commit at the end.

@@ -257,6 +257,14 @@ each scenario instead of flushing the entire database. Tests run much quicker,
however HTTP server is not started and therefore web browser automation is
not available.

``--runner``
*******************
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 the ***** only as long as the section title.

``--runner``
*******************

Allows to override the test runner class that Behave uses by default. e.g.,
Copy link
Member

Choose a reason for hiding this comment

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

The "e.g.," is lost at the end of this line here. Why not remove it here and use it properly in the next sentence?

*******************

Allows to override the test runner class that Behave uses by default. e.g.,
this will allow you to customize where your feature files are located.
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest:

This will allow you, e.g., to customize ...

parser.add_argument(
'--runner', action='store', dest='runner_class',
default='behave.runner.Runner', type=valid_python_module,
help='Tells Behave to use a specific runner. (default: %(default)s)',
Copy link
Member

Choose a reason for hiding this comment

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

This is actually the Behave runner, not any of those we provide in this project. I'm fascinated!

Copy link
Member

Choose a reason for hiding this comment

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

While I start liking this PR more and more I now start worrying that wondering whether 😏 this feature should actually go into Behave. What do you think about that? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I'm looking forward to your feedback here. The behave command actually has no -r --runner option, currently. Maybe it would be desirable to have that possibility also for plain Behave, not only tied to using it with Django, hmm?

I would still merge your efforts here, probably, but:

  • name the option appropriately --behave-runner (as you had it before) and
  • open a new issue to discuss migrating the feature to the Behave code base.

I hope that makes sense.

@@ -5,4 +5,4 @@
__email__ = 'mixxorz@gmail.com'
__url__ = 'https://github.com/behave/behave-django'
__license__ = 'MIT'
__version__ = '1.3.0'
__version__ = '1.3.1'
Copy link
Member

Choose a reason for hiding this comment

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

That's more than just a bug fix or something. Let's make it 1.4.0, what do you think?

@kingbuzzman
Copy link
Contributor Author

@bittner I think this makes all the sense in the world, if i move this code over to https://github.com/behave/behave, then there is no need for this PR

@kingbuzzman
Copy link
Contributor Author

kingbuzzman commented Nov 25, 2019

I tested the code in master with the modification ive done to https://github.com/behave/behave-django and it looks good. Closing this PR.

Code has been moved to behave/behave#795

@kingbuzzman
Copy link
Contributor Author

kingbuzzman commented Nov 23, 2020

@bittner so.. i think for the time being, maybe we can just add this here. When @jenisys gets around to merging the other PR we can delete it.. what do you think?

@bittner
Copy link
Member

bittner commented Nov 23, 2020

Let me try to get hold of @jenisys a last time. This should be possible. He has merged things before.

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

Successfully merging this pull request may close these issues.

None yet

3 participants