Re: [PATCH 17/17] docs: Make API document versioned
On Sun, 2018-11-11 at 10:36 +1100, Daniel Axtens wrote:
> Stephen Finucane writes:
>
> > On Fri, 2018-11-09 at 00:30 +1100, Daniel Axtens wrote:
> > > I've now very lightly skimmed the preceeding commits to the yaml file.
> > > I don't really mind if it goes in, but I'd expect it to bitrot
> > > pretty quickly if there isn't one or more of the following:
> > > - real uptake of it
> > > - it being almost entirely computer-generated
> > > - automatic validation/regression testing of the spec against the code
> > >in e.g. tox.
> >
> > Agreed. I have a series in the works to generate this using the
> > openapi-core library [1][2]. My thinking is that we'll use the schema
> > to validate things both server side and client side, starting with git-
> > pw (and extending to pwclient once we decide how we're moving forward
> > with that). This should make testing of clients a heck of a lot easier
> > (currently I need to spin up a Patchwork instance or rely on unit
> > tests) and it also opens the door for automatic client API code
> > generation in the future. Having worked with git-pw for a while now, I
> > think that's a increasingly good idea.
> >
> > > This commit makes me more uncomfortable, I think it's going to end up
> > > being more trouble than it's worth vs just keeping a file for each
> > > schema. The old ones should stay static, right?
> >
> > My thinking for this was to avoid duplication and avoid storing auto-
> > generated code in version control. The documentation patches I plan to
> > send later include a Sphinx extension to call the 'generate_schema'
> > command locally before docs. Ditto for testing. I can store the
> > generated files in-tree if this would be preferred though.
>
> Well it's not really autogenerated if you have to do a bunch of manual
> editing to the generated file :) And I'm guessing you won't be able to
> auto-generate the fully templated file either?
Ah, I meant the template was there to avoid duplication and I wasn't
storing the output of the 'generate_schema' call because it was auto-
generated. It's not possible to generate the schema automatically, with
or without templates, because the tooling just isn't there yet.
> I also don't really find the idea of having API descriptions be quite
> similar between versions to be that big of a deal - or at least, not as
> big a deal as the added complexity of deduplicating via templating.
>
> My gut feeling is that you're making a rod for your own back with the
> added complexity - but I haven't got a good grasp on the grand scheme
> you have in mind, so maybe it works out simpler in the long run.
I do have an additional use for Jinja2, in the form of including the
sample calls/responses introduced in that other patch series. Let's
stick with this for now, until I have that fleshed out. If it turns out
we can get away without Jinja2 for this, we can drop the templating and
go with static. Sound fair?
Stephen
> Regards,
> Daniel
>
> > > Anyway, it's not a hard NAK, but you should include jinja2 in a
> > > requirements file, unless it's pulled in by something else - and even
> > > then it wouldn't hurt to note it.
> >
> > Fair. Sphinx is pulling Jinja2 in but I can note this explicitly.
> >
> > Stephen
> >
> > [1] https://github.com/p1c2u/openapi-core
> > [2] https://pypi.org/project/openapi-core/
> >
> > > Regards,
> > > Daniel
> > >
> > > Stephen Finucane writes:
> > >
> > > > OpenAPI doesn't appear to support versioning natively, suggesting
> > > > instead that separate documents are kept. Rather than doing this
> > > > manually, let's use a templating tool - Jinja2, in this case - to
> > > > generate these document for us from a single master document.
> > > >
> > > > Signed-off-by: Stephen Finucane
> > > > ---
> > > > .gitignore| 1 +
> > > > docs/api/schemas/generate_schema.py | 29 +
> > > > .../schemas/{patchwork.yaml => patchwork.j2} | 31 +--
> > > > 3 files changed, 59 insertions(+), 2 deletions(-)
> > > > create mode 100644 docs/api/schemas/generate_schema.py
> > > > rename docs/api/schemas/{patchwork.yaml => patchwork.j2} (98%)
> > > >
> > > > diff --git a/.gitignore b/.gitignore
> > > > index a33d1029..da53b382 100644
> > > > --- a/.gitignore
> > > > +++ b/.gitignore
> > > > @@ -44,6 +44,7 @@ htmlcov/
> > > >
> > > > # Sphinx stuff
> > > > /docs/_build
> > > > +/docs/api/schemas/v*
> > > >
> > > > # Selenium test artifacts
> > > > /selenium.log
> > > > diff --git a/docs/api/schemas/generate_schema.py
> > > > b/docs/api/schemas/generate_schema.py
> > > > new file mode 100644
> > > > index ..65d4ed82
> > > > --- /dev/null
> > > > +++ b/docs/api/schemas/generate_schema.py
> > > > @@ -0,0 +1,29 @@
> > > > +#!/usr/bin/env python3
> > > > +
> > > > +import os
> > > > +
> > > > +import jinja2
> > > > +
> > > > +ROOT_DIR = os.path.dirname(os.path.realpath(__file__))
> > > > +VERSIONS = [(1, 0),
Re: [PATCH 00/13] Start generating API examples from tests
On Sun, 2018-11-11 at 12:00 +1100, Daniel Axtens wrote: > Hi Stephen, > > > As part of the OpenAPI work, I realized I wanted to be able to show > > actual API requests and responses for the documentation. We could > > generate these manually but doing so would be tedious and would need to > > be redone every time the API changed. There is a better way: generate > > these things from the tests. We know that the requests and responses > > must be correct because we're actually testing against them. It also > > provides an opportunity to examine our test coverage and fill in gaps, > > of which there are a few. All of these samples are currently unused but > > they will be used once we merge in the OpenAPI work. > > > > Note that some of the changes are bigger than we'd like. That's because > > some tests were actually testing multiple things. This reduced the LoC > > necessary but meant we didn't have a single request/response to save. > > (Sorry, I would test this myself, but what with me being in an airport > and this potentially interacting with other series, I figure it's best > to just ask this time...) > > What's the impact on the time to run tests with this? I notice it splits > up a lot of tests; does that make the already painfully slow test suite > any slower? There is an impact but it's very slight. Before: $ docker-compose run --rm web tox -e py27-django111 patchwork.tests.api -- Ran 44 tests in 2.604s OK After: $ docker-compose run --rm web tox -e py27-django111 patchwork.tests.api . -- Ran 97 tests in 3.980s OK Personally, I think this is acceptable as the samples are helpful and the tests probably should have been written in this more modular fashion the first day. As a general rule of thumb too, the REST API tests are very fast, once you exclude the time taken to set up the testing database (which is constant and slow). The slowest tests are the ones for views, which require a lot more of Django's machinery. We should probably try to optimize those. Stephen > Regards, > Daniel > > > Stephen Finucane (13): > > tests: Add 'store_samples' decorator > > tests: Add 'store_samples' decorator to 'test_bundle' > > tests: Add 'store_samples' decorator to 'test_project' > > tests: Add 'store_samples' decorator to 'test_people' > > tests: Add 'store_samples' decorator to 'test_user' > > tests: Add 'store_samples' decorator to 'test_patch' > > tests: Add 'store_samples' decorator to 'test_cover' > > tests: Add 'store_samples' decorator to 'test_series' > > tests: Add 'store_samples' decorator to 'test_comment' > > tests: Add 'store_samples' decorator to 'test_check' > > tests: Add tests for '/events' resource > > tests: Add 'store_samples' decorator to 'test_event' > > signals: Fix 'series-completed' event > > > > .gitignore | 1 + > > patchwork/signals.py| 21 +++- > > patchwork/tests/api/test_bundle.py | 76 +++-- > > patchwork/tests/api/test_check.py | 54 ++--- > > patchwork/tests/api/test_comment.py | 57 ++ > > patchwork/tests/api/test_cover.py | 51 ++--- > > patchwork/tests/api/test_event.py | 165 > > patchwork/tests/api/test_patch.py | 145 > > patchwork/tests/api/test_person.py | 37 +-- > > patchwork/tests/api/test_project.py | 110 ++- > > patchwork/tests/api/test_series.py | 65 +++ > > patchwork/tests/api/test_user.py| 65 +-- > > patchwork/tests/api/utils.py| 93 > > patchwork/tests/test_events.py | 33 +- > > patchwork/tests/utils.py| 10 ++ > > 15 files changed, 808 insertions(+), 175 deletions(-) > > create mode 100644 patchwork/tests/api/test_event.py > > create mode 100644 patchwork/tests/api/utils.py > > > > -- > > 2.17.2 > > > > ___ > > Patchwork mailing list > > Patchwork@lists.ozlabs.org > > https://lists.ozlabs.org/listinfo/patchwork ___ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
Re: [PATCH] Don't passthrough 'Content-Type: multipart/signed' header
- Original Message -
> From: "Stephen Finucane"
> To: patchwork@lists.ozlabs.org
> Cc: "Stephen Finucane" , "Veronika Kabatova"
>
> Sent: Sunday, November 4, 2018 3:27:04 PM
> Subject: [PATCH] Don't passthrough 'Content-Type: multipart/signed' header
>
> We don't GPG signatures, therefore this header is incorrect. Stop
> passing it through.
>
> Test for the other dropped header are also included.
>
> Signed-off-by: Stephen Finucane
> Cc: Veronika Kabatova
> Closes: #221
> ---
> patchwork/tests/test_mboxviews.py | 15 +++
> patchwork/views/utils.py | 6 ++
> 2 files changed, 21 insertions(+)
>
> diff --git a/patchwork/tests/test_mboxviews.py
> b/patchwork/tests/test_mboxviews.py
> index 50444d65..87c75eca 100644
> --- a/patchwork/tests/test_mboxviews.py
> +++ b/patchwork/tests/test_mboxviews.py
> @@ -111,6 +111,21 @@ class MboxHeaderTest(TestCase):
> header = 'List-Id: Patchwork development
> '
> self._test_header_passthrough(header)
>
> +def _test_header_dropped(self, header):
> +patch = create_patch(headers=header + '\n')
> +response = self.client.get(reverse('patch-mbox', args=[patch.id]))
> +self.assertNotContains(response, header)
> +
> +def test_header_dropped_content_transfer_encoding(self):
> +"""Validate dropping of 'Content-Transfer-Encoding' header."""
> +header = 'Content-Transfer-Encoding: quoted-printable'
> +self._test_header_dropped(header)
> +
> +def test_header_dropped_content_type_multipart_signed(self):
> +"""Validate dropping of 'Content-Type=multipart/signed' header."""
> +header = 'Content-Type: multipart/signed'
> +self._test_header_dropped(header)
> +
> def test_patchwork_id_header(self):
> """Validate inclusion of generated 'X-Patchwork-Id' header."""
> patch = create_patch()
> diff --git a/patchwork/views/utils.py b/patchwork/views/utils.py
> index 3c5d2982..1da1aaab 100644
> --- a/patchwork/views/utils.py
> +++ b/patchwork/views/utils.py
> @@ -84,8 +84,14 @@ def _submission_to_mbox(submission):
>
> orig_headers = HeaderParser().parsestr(str(submission.headers))
> for key, val in orig_headers.items():
> +# we set this ourselves
> if key == 'Content-Transfer-Encoding':
> continue
> +# we don't save GPG signatures described in RFC1847 [1] so this
> +# Content-Type value is invalid
> +# [1] https://tools.ietf.org/html/rfc1847
> +if key == 'Content-Type' and val == 'multipart/signed':
> +continue
> mail[key] = val
>
Good catch!
Acked-by: Veronika Kabatova
> if 'Date' not in mail:
> --
> 2.19.1
>
>
___
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
