Enhancement: add pytest-bdd parser support #29

bradsbrown · 2020-02-17T20:48:50Z

I think this one is "most of the way" there, though it did reveal some of the weaknesses of having left behind the class structure for file parsing -- it makes it much less clear what needs to differ between behave- and pytest-bdd-parsed files, and adds some (potentially unnecessary) complexity to the work of keeping the two in-sync.

Consider this a "spike verging on mergeability", but if it leads to larger discussions/changes, I'm ok with that as well.

rbcasperson

I didn't dive into too much because I trust that you can figure out the pytest-bdd specific stuff. Overall, I see lots of duplication, so I wonder if this can pivot a bit. I think we could make our own classes for Feature, Scenario, etc that have all the information we need, and then pass those sanitized objects into our writing to rst logic. Maybe you were thinking that anyway. In other words, use whatever parser to parse the file, then use that parsed file to create the objects we need. So for example, our Feature object would have an optional examples value. Behave would never set that, but pytest-bdd might. Then "get info we need" and "turn that info into rst and write it" can be two clearly separate tasks.

sphinx_gherkindoc/cli.py

dgou · 2020-02-18T02:14:24Z

I have the same reaction as Ryan.
It feels that separating the parser from the rest of the logic would be a good way to isolate the parser variations from the backend processing (which is mostly formatting).
I think of it kinda like how coverage worked, extract into a neutral data structure, then process that data structure. That would also allow for other languages to feed in to the backend by just writing out serialized data (maybe JSON, but whatever).
I say this all having only looked at Ryan's comments and not yet having looked at the code. :-)

bradsbrown · 2020-02-18T02:22:11Z

I’m glad to hear two votes for moving into some cleaner class-based structure. That’s more or less where it felt like this needed to go, once I got to this point. Just wanted to hear that independently confirmed before I put in the work for the next round.

dgou

Misc quickly reviewed comments.

dgou · 2020-02-18T02:16:30Z

pyproject.toml

@@ -15,6 +15,7 @@ Sphinx = ">=1.3"
 sphinx_rtd_theme = ">=0.3.1"
 behave = ">=1.2.6"
 recommonmark = ">=0.4.0"
+pytest-bdd = {git = "https://github.com/rbcasperson/pytest-bdd.git", rev = "scenario-descriptions", optional = true}


Huh, I hope this reference to a personal repo is something that will evolve into a JGT repo at some point?
(tiny github PR review UI syndrome might be at work here too)

I am still concerned about this 'personal repo' being used here...

So I've looked a bit into this. poetry does support most of the more advanced ways you can specify a package, but doesn't appear to properly handle parsing/passing a PR refspec down to pip/git. Our options, as best I can determine, are:

sit on this addition until Ryan's PR merges (it's been awaiting review since Dec 3)

rework the pytest-bdd parser class to strip out scenario descriptions for now and just add support for scenario descriptions back later when the PR merges

run with the personal branch from which the PR is sourced until that PR merges (then come back and return our ref to the "official" pytest-bdd source)

Working on a team that could benefit today from the full range of what this work currently offers, I'm obviously biased toward option (3) there.

Isn't there maybe a 4th option?
What about using the stock pytest-bdd parser as is, in which case the extra functionality won't come out of the stock parser, but then anyone who wanted to layer in Ryan's PR after the install would get it and the sphinx-gherkindoc part would be ready?
(Sorry, this is a dash-off quick half-baked thought as I didn't have any more time today at lunch)

dgou · 2020-02-18T02:18:55Z

sphinx_gherkindoc/cli.py

+        if args.pytest_parser:
+            from .pytest_bdd_writer import feature_to_rst
+        else:
+            from .behave_writer import feature_to_rst  # type: ignore


Sadly from a historical perspective, behave is the default if nothing else is specified. Much as I would like to require the parser (well, the syntax, technically) to be actively declared in all cases, that would break existing clients.
Unless we go to a full-on major version number change. :-)

dgou · 2020-02-18T02:20:37Z

sphinx_gherkindoc/pytest_bdd_writer.py

+        output_file.blank_line()
+        examples(scenario, feature)
+
+    return output_file


Ugh, I can't tell just how duplicated this is using the GIthub PR review UI alone.
I'll have to pull the PR and do some diffing and what not.
(So perhaps not tonight unfortunately, hopefully tomorrow)

tests/basic.feature

dgou · 2020-02-18T02:24:28Z

tests/conftest.py

+    basic_feature = tmp_path / "basic.feature"
+    test_dir = pathlib.Path(__file__).parent
+    with open(test_dir / "basic.feature") as feature_fo:
+        basic_feature.write_text(feature_fo.read())


Not sure if that is the same as, or subtly different from, https://docs.python.org/3/library/shutil.html#shutil.copyfile

Question about shutil.copyfile still stands...

both of these were lift-and-shift, so I didn't look carefully, and missed the comment(s) in the larger rewrite, but I just tested and. shutil.copyfile works fine here (and below), so I'll make that change.

dgou · 2020-02-18T02:24:46Z

tests/conftest.py

+    tags_feature = tmp_path / "tags.feature"
+    test_dir = pathlib.Path(__file__).parent
+    with open(test_dir / "tags.feature") as feature_fo:
+        tags_feature.write_text(feature_fo.read())


ditto comment re: https://docs.python.org/3/library/shutil.html#shutil.copyfile here too?

ditto here too.

dgou · 2020-02-18T02:33:28Z

tests/test_pytest_writer.py

+    #  and tag lines for all the same "words"
+
+    # First, compare non-tag lines
+    actual_without_tags = [x for x in actual if tag_text not in x]


Too bad we're not using more_itertools :-)
https://more-itertools.readthedocs.io/en/stable/api.html#more_itertools.partition

sphinx_gherkindoc/writer.py

tests/basic.feature

dgou · 2020-02-25T13:06:15Z

(Edited to add this is with python 3.7.6)
(Edited to add poetry --version -> Poetry 0.12.17 )
On master, I spun up a new venv, ran the triumvirate (envsetup, self check, run tests) all golden.
Killed that venv:

$ git pr 29 upstream
From github.com:jolly-good-toolbelt/sphinx_gherkindoc
 * [new ref]         refs/pull/29/head -> pr/29
Switched to branch 'pr/29'

spun up a new venv and when running envsetup:

$ ./env_setup.py 
In: /home/dwp/code/sphinx_gherkindoc
Setting up Virtual Environment: /home/dwp/.virtual_envs/8e9d7ef53151dc3

Collecting pip<19
  Using cached pip-18.1-py2.py3-none-any.whl (1.3 MB)
Installing collected packages: pip
  Attempting uninstall: pip
    Found existing installation: pip 20.0.2
    Uninstalling pip-20.0.2:
      Successfully uninstalled pip-20.0.2
Successfully installed pip-18.1
Installing dependencies from lock file
                                  
[NonExistentKey]   
'Key "hashes" does not exist.'  
                                  
install [--no-dev] [--dry-run] [-E|--extras EXTRAS] [--develop DEVELOP]

Traceback (most recent call last):
  File "./env_setup.py", line 59, in <module>
    main()
  File "./env_setup.py", line 55, in main
    env_setup(args.verbose)
  File "./env_setup.py", line 40, in env_setup
    execute_command_list(__commands_to_run, verbose=verbose)
  File "./env_setup.py", line 31, in execute_command_list
    subprocess.run(shlex.split(command), check=True)
  File "/home/linuxbrew/.linuxbrew/opt/python/lib/python3.7/subprocess.py", line 512, in run
    output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command '['poetry', 'install', '-E', 'pytest-bdd']' returned non-zero exit status 1.

dgou · 2020-02-25T13:49:28Z

Upgraded poetry to 1.0.3 and the previous error went away.
Got this warning:

Installing dependencies from lock file
Warning: The lock file is not up to date with the latest changes in pyproject.toml. You may be getting outdated dependencies. Run update to update them.

dgou · 2020-02-26T01:56:35Z

Pulled and ran against SNBN repo.
Output files in docs and _docs were the same.
(default to behave and it did, well, behave itself well)

dgou

Back to Brad with a few hopefully minor things.
Overall the code looks good.
I have to self-disclaim that the pytest-bdd specific parts are beyond my familiarity, so I leave it to ye lovers of pytest to review that with a keener eye.

dgou · 2020-02-27T18:10:49Z

So I had a few minutes at lunch today, and I tried an experiment.
Since I don't have a ready-made pytest-bdd based product, I did the following:

spun up a fresh venv
ran the triumvirate scripts -all good
did poetry build to make an installable package.

To semi simulate a potential user environment:

pip uninstall pytest-bdd
pip install pytest-bdd # to get the release version from PyPI as it would be already installed in the user's environment for someone wanting to add in this doc-ability
`pip install 'dist/sphinx_gherkindoc-3.4.0.tar.gz[pytest-bdd]'
Noticed that Ryan's PR branch is not installed
./run_tests.py
--> Various failures such as:

self = <sphinx_gherkindoc.parsers.pytest_bdd.Scenario object at 0x7fed40451490>, key = 'description'

    def __getattr__(self, key):
        """Grab attribute from wrapped class, if present."""
>       return getattr(self._data, key)
E       AttributeError: 'Scenario' object has no attribute 'description'

../../.virtual_envs/459314548daf648/lib/python3.7/site-packages/sphinx_gherkindoc/parsers/base.py:12: AttributeError

So it seems, and I am not sure if my experiment is fully valid, that this PR could have two sad consequences, depending on how a user's environment is configured:

pytest-bdd as released is installed first, then the sphinx-gherkindoc installed with the pytest-bdd option doesn't operate as exected and the --parser pytest-bdd option will break strangely.
sphinx-gherkindoc with the pytest-bdd option is installed first, and masquerades as 3.2.1 when it is not quite what it says.

So while one might argue that option 2 is OK, that really only applies if there are no other PRs merged and no other pytest-bdd releases done before Ryan's is.

And, more importantly, all of this working on not depends on the order that a user installs their packages, which is very fragile and would be downright mysterious to anyone who just wants to get some stuff documented.

I think if Ryan's PR had a version dink in it, that might help some, but even then, if pytest-bdd does a new release without that PR, we're back again to having to have a very specific ordering of installation for things to work right.

That leaves me thinking that the best way to move forward with this PR is not to have the the pytest-bdd option install Ryan's PR and be operable without it, but if someone does come along and add in Ryan's PR after the fact, this code will be able to leverage that without requiring any install/uninstall of sphinx-gherkindoc.

Tweak test cases such that the pytest scenarios pass with the latest pytest-bdd release, without dropping any coverage around the behave tests.

sphinx_gherkindoc/parsers/base.py

tests/test_pytest_writer.py

dgou

Took a peep at the latest changes in toto and it seems good.
Want to test against real repo doc build before signing off, that'll be tomorrow morning I hope.

dgou

Cannot say I went very deep in to the pytest-bdd part of things not having a real world use case for it.
However, I did just run master and this PR on my real world behave test case and everything built exactly the same except for the usual diff in the environment.pickle file.

tl;dr LGTM!

brolewis

Looks good to me. Utilize all the engines!

bradsbrown added 10 commits February 17, 2020 13:51

Enhancement: add optional pytest-bdd

1249ed4

Enhancement: split out behave logic

3cf2e47

Enhancement: add pytest_bdd logic

40744bc

Enancement: cli update

6e2dde2

Enhancement: handle feature examples

42ea1bb

Enhancement: split out tests

d4bd6ae

Enhancement: fix output and tests

2e046be

Enhancement: update from master

1834727

Enhancement: test updates

e1a552c

Enhancement: version bump

10a180f

bradsbrown assigned dgou, brolewis, arounkles, rbcasperson, sdutton-personal and ecedmondson Feb 17, 2020

bradsbrown added 3 commits February 17, 2020 14:59

Enhancement: fix url_parser handling

feefe0c

Enhancement: dataclass >> namedtuple for py3.6 compat

01a7a02

Enhancement: add extras to install

c7c62ed

rbcasperson reviewed Feb 18, 2020

View reviewed changes

sphinx_gherkindoc/cli.py Show resolved Hide resolved

dgou reviewed Feb 18, 2020

View reviewed changes

bradsbrown added 7 commits February 20, 2020 15:27

Enhancement: drop separate writers

e643721

Enhancement: implement parser classes

3d72cb0

Enhancement: restore unified writer, update cli

4351cb8

Enhancement: realign tests

33e2298

Enhancement: drop moved utils entries

b6b36b2

Enhancement: use choices for cli arg

d3680e6

Merge branch 'master' into pytest_bdd

962fc36

dgou reviewed Feb 24, 2020

View reviewed changes

sphinx_gherkindoc/writer.py Show resolved Hide resolved

dgou reviewed Feb 24, 2020

View reviewed changes

tests/basic.feature Show resolved Hide resolved

bradsbrown requested review from dgou and rbcasperson February 24, 2020 13:48

dgou assigned bradsbrown and unassigned dgou Feb 25, 2020

Enhancement: update lockfile

c849c69

dgou requested changes Feb 26, 2020

View reviewed changes

Enhancement: use shutil.copyfile

8380ca7

bradsbrown requested a review from dgou February 26, 2020 15:46

bradsbrown added 5 commits April 16, 2020 10:52

make ryan's branch a dev dep only

96fc6d2

Adjust tests for pytest-bdd

ffe1dc0

Tweak test cases such that the pytest scenarios pass with the latest pytest-bdd release, without dropping any coverage around the behave tests.

Merge branch 'master' into pytest_bdd

26898da

Support step.step_type in pytest parser

54658a6

linting fixes

24e7948

rbcasperson reviewed Apr 16, 2020

View reviewed changes

sphinx_gherkindoc/parsers/base.py Show resolved Hide resolved

tests/test_pytest_writer.py Outdated Show resolved Hide resolved

tests/test_pytest_writer.py Outdated Show resolved Hide resolved

tests/test_pytest_writer.py Show resolved Hide resolved

descriptive docstrings, drop dead code

07578be

bradsbrown requested a review from rbcasperson April 16, 2020 17:30

rbcasperson approved these changes Apr 16, 2020

View reviewed changes

tests/test_pytest_writer.py Show resolved Hide resolved

rbcasperson removed their assignment Apr 16, 2020

dgou reviewed Apr 17, 2020

View reviewed changes

dgou approved these changes Apr 17, 2020

View reviewed changes

brolewis approved these changes Apr 17, 2020

View reviewed changes

brolewis merged commit 91414e2 into jolly-good-toolbelt:master Apr 17, 2020

bradsbrown mentioned this pull request Apr 22, 2020

Some tweaks to the pytest-bdd support. bradsbrown/sphinx_gherkindoc#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement: add pytest-bdd parser support #29

Enhancement: add pytest-bdd parser support #29

bradsbrown commented Feb 17, 2020

rbcasperson left a comment

dgou commented Feb 18, 2020

bradsbrown commented Feb 18, 2020

dgou left a comment

dgou Feb 18, 2020

dgou Feb 26, 2020

bradsbrown Feb 26, 2020

dgou Feb 26, 2020

dgou Feb 18, 2020

dgou Feb 18, 2020

dgou Feb 18, 2020

dgou Feb 26, 2020

bradsbrown Feb 26, 2020

dgou Feb 18, 2020

dgou Feb 26, 2020

dgou Feb 18, 2020

dgou commented Feb 25, 2020 •

edited

Loading

dgou commented Feb 25, 2020

dgou commented Feb 26, 2020

dgou left a comment

dgou commented Feb 27, 2020

Noticed that Ryan's PR branch is not installed

dgou left a comment

dgou left a comment

brolewis left a comment

Enhancement: add pytest-bdd parser support #29

Enhancement: add pytest-bdd parser support #29

Conversation

bradsbrown commented Feb 17, 2020

rbcasperson left a comment

Choose a reason for hiding this comment

dgou commented Feb 18, 2020

bradsbrown commented Feb 18, 2020

dgou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgou commented Feb 25, 2020 • edited Loading

dgou commented Feb 25, 2020

dgou commented Feb 26, 2020

dgou left a comment

Choose a reason for hiding this comment

dgou commented Feb 27, 2020

To semi simulate a potential user environment:

Noticed that Ryan's PR branch is not installed

dgou left a comment

Choose a reason for hiding this comment

dgou left a comment

Choose a reason for hiding this comment

brolewis left a comment

Choose a reason for hiding this comment

dgou commented Feb 25, 2020 •

edited

Loading