collective / sphinxcontrib-httpexample

Adds example directive for sphinx-contrib httpdomain
23 stars 20 forks source link

Snapshot testing and builder fixes and improvements #20

Closed dokai closed 6 years ago

dokai commented 6 years ago

Sorry about the rather large pull-request, however the individual commits each introduce an isolate change. My overall goal was to bring the project to a point where we can use it successfully to document our API examples.

In order to improve the developer experience I've introduced snapshot testing using snapshottest which is similar in concept to snapshot testing in Jest. With the expected outputs managed automatically with snapshottest it is easy to refactor the code and update the snapshots with

./bin/tox --develop -- --snapshot-update

Additionally, tox will now forward command line parameters to pytest so the full diff of any failing snapshot can be easily viewed with

./bin/tox --develop -- -vv

The other testing related change is to use @pytest.mark.parametrize to reduce the boiler plate in writing tests for new examples.

The rest of the commits either fix issues with quoting the commands and their parameters or enhance their output. For httpie I've switched to using redirected input to pass in the request payload to improve readability and avoid the issues with trying to transform the payload into key-value parameters.

dokai commented 6 years ago

It seems the Travis job has a fairly old pytest-2.9.2 version. Is there anything preventing it being updated to the latest?

datakurre commented 6 years ago

@dokai Thanks again. I appreciate that you solved how to make it easier to add new tests.

I’ll take some time to learn that snapshot approach myself, fix my Nix setup (hint: Python has not been the nicest language with due to imperative setup.py), and merge this.

I might also move most of setup.py into setup.cfg (supported by setuptools for some time). Hopefully requirement for new setuptools version is not an issue. (Release will include wheel anyway.)

datakurre commented 6 years ago

@dokai Don’t waste your time with Travis :) That setup has been my playground so, I’ll fix that.

dokai commented 6 years ago

@datakurre Thanks, let me if know if I can clarify anything or help improve the PR. I'm not familiar with Nix myself beyond its existence and for local development I've simply run the tests with ./bin/tox --develop to verify they pass.

Additionally, I've confirmed that it works with our project documentation (however we're mostly interested in cURL/httpie examples).

datakurre commented 6 years ago

Thanks. I'll release a new version shortly.

Although, I had and expected issue with snapshottests:

You expected python requests command builder to just work with your more complex new examples. Unfortunately, I had intentionally (in TDD way) make it only work with simple JSON data until use case for a more complex data emerges. When you just expected it to work, you had recorded broken requests example as valid test result. Well, it's better now :) (Fun example for generating Python code examples with AST when you need it...)

dokai commented 6 years ago

@datakurre: Thanks for working on finalizing the PR. I overlooked the python-requests examples as we don't include those in our docs.

Looking at that section more closely I wonder about the original use cases you had that led you to implement the AST based handling for the entire JSON payload. Given that the input data is by definition already JSON encoded did you consider passing that through as-is to requests and perhaps just pretty-printing it in the generated code?

For example, instead of

requests.post('https://api.com/path', headers={'Content-Type':'application/json'}, json={'foo':'bar'}

which requires first parsing {"foo":"bar"} into a Python object and then have requests serialize it again we could just pass the JSON string as-is with the data parameter.

requests.post('https://api.com/path', headers={'Content-Type':'application/json'}, data='{"foo":"bar"}'

It might make sense to extract the data into a variable to allow pretty printing for better readability, e.g.

payload = """
{
  "foo": "bar",
  "bar": [1, 2, 3]
}
"""
requests.post('https://api.com/path', headers={'Content-Type':'application/json'}, data=payload)
datakurre commented 6 years ago

I admit I didn’t think about data=payload approach. I also overlooked readability because our initial payloads were smaller.

AST was fun to learn, but I consider this when I return this next time. Maybe request examples could be one liners with small payload (like small PATCH), but literal JSON values for larger ones for better readability. Thanks for the suggestion!