Fix existing examples #387

JP-Ellis · 2023-09-15T06:46:50Z

This PR resolves #370.

When first picking up pact-python, I immediately ran into some issues with running the examples.

This PR migrates the examples into a single test suite to be executed with hatch run example. This will (optionally) spin up the pact broker container, and maintain it across all consumer and provider examples.

The idea for this example is to have the consumer run first and publish the contracts, and then to verify the providers against these contracts.

In doing so, I have also attempted to document rather explicitly the way the examples are written and how they might be used. It is not designed to replace the docs in any way, but hopefully will provide a much more hand-lead introduction for pact-python.

pyproject.toml

YOU54F · 2023-09-19T16:08:34Z

docker not available on macos runners in cirrus ci or github actions.

licensing reasons on gha and because of apple virtulization restrictions for cirrus ci, which doesn't permit nested virtualization.

I took a trip down ruby executable package lane, to look at packaging up the pact broker, as part of our pact-ruby-standalone bundle - can we make a pact broker available in the CI workflows, easily without docker. (we could just install ruby and pull down the pact broker which would be the easiest thing, but I do like a random adventure that scratches a few itches)

It worked well for linux and macos, but for windows, is a no go, as it relies on natives gems for the pact broker, of which the traveling-ruby package doesn't provide.

There are some windows projects that support packaging windows apps, with native extensions

pact-foundation/pact-ruby-standalone#118

and also package into a single file.

I thought it would be nice for macos and linux as well.

That lead me to fixing up ruby-packer

pmq20/ruby-packer#177

Mmm that was okay but there were issues loading native gems, which cropped up occasionally and I couldn't pin it down, so ended up thinking, why not try to roll my own.

In doing so, I stumbled across a random SO post here

https://stackoverflow.com/questions/76579841/package-standalone-rubygems-for-a-macos-app/76741361#76741361

I documented all my findings here

https://gist.github.com/YOU54F/3775e66e6090e0371c11601e6b75c305

note if we got some hardware for self hosted runners, we could runner docker natively on mac, and not be using macos in a vm (with cirrus)

YOU54F · 2023-09-19T16:16:23Z

I also made changes in my PR to support switching between using a hosted OSS broker that PactFlow provides (for ci/testing/poking around)

59e2ef6

I was hitting rate limits as the CI tests would run against x python versions on 3 OS's, and 2 arches, whilst using fix app version and branches, and the same consumer/provider names.

We would be limited cos we are smashing the same endpoint with the same data xD

adding the commit and branch from the actual branch/commit of the repo along with an identifier for --<ci_system> would be useful to avoid clashing, rate limiting and identification, back, in case of discrepancies between any of the generated artefacts (pact files in the examples case)

YOU54F · 2023-09-19T18:44:23Z

Odd error in CI on Cirrus for MacOS and Linux on python 3.8 (which I can't replicate locally)

python 3.8 linux https://cirrus-ci.com/task/5818697332293632
python 3.8 mac https://cirrus-ci.com/task/6100172309004288

_multicall
    res = hook_impl.function(*args)
  File "/tmp/cirrus-ci-build/examples/conftest.py", line 24, in pytest_addoption
    action=argparse.BooleanOptionalAction,
AttributeError: module 'argparse' has no attribute 'BooleanOptionalAction'

seems to relate to

https://nono.ma/attribute-error-module-argparse-has-no-attribute-boolean-optional-action

odd how we don't see it in github actions! oh right this will make sense, we aren't running the examples on anythign other than the latest version, whereas we are running it across all the versions in cirrus

https://github.com/pact-foundation/pact-python/actions/runs/6233729823/workflow#L56

Might just want to conditionally pass in diff args based on the python version

note i get the same results with Python 3.8.18 , if running the cirrus macos task locally with cirrus cli/tart, but running it on my own machine (rather than in the vm) results seems to be fine ( well bar an error I think you mentioned to me earlier on the message provider)

That's cool though, I can help you work out why that isn't working (it's probably something simple)

timing wise, its really good compared to before. I've not dug into the example code setup too much today, just getting it running from the command line 👍🏾

Python 3.9 errors in macos / linux on cirrus ci

https://cirrus-ci.com/task/5255747378872320

==================================== ERRORS ====================================
_________ ERROR collecting examples/tests/test_01_provider_fastapi.py __________
/root/.local/share/hatch/env/virtual/pact-python/XZqP7kBn/pact-python/lib/python3.9/site-packages/_pytest/runner.py:341: in from_call
    result: Optional[TResult] = func()
/root/.local/share/hatch/env/virtual/pact-python/XZqP7kBn/pact-python/lib/python3.9/site-packages/_pytest/runner.py:372: in <lambda>
    call = CallInfo.from_call(lambda: list(collector.collect()), "collect")
/root/.local/share/hatch/env/virtual/pact-python/XZqP7kBn/pact-python/lib/python3.9/site-packages/_pytest/python.py:531: in collect
    self._inject_setup_module_fixture()
/root/.local/share/hatch/env/virtual/pact-python/XZqP7kBn/pact-python/lib/python3.9/site-packages/_pytest/python.py:545: in _inject_setup_module_fixture
    self.obj, ("setUpModule", "setup_module")
/root/.local/share/hatch/env/virtual/pact-python/XZqP7kBn/pact-python/lib/python3.9/site-packages/_pytest/python.py:310: in obj
    self._obj = obj = self._getobj()
/root/.local/share/hatch/env/virtual/pact-python/XZqP7kBn/pact-python/lib/python3.9/site-packages/_pytest/python.py:528: in _getobj
    return self._importtestmodule()
/root/.local/share/hatch/env/virtual/pact-python/XZqP7kBn/pact-python/lib/python3.9/site-packages/_pytest/python.py:617: in _importtestmodule
    mod = import_path(self.path, mode=importmode, root=self.config.rootpath)
/root/.local/share/hatch/env/virtual/pact-python/XZqP7kBn/pact-python/lib/python3.9/site-packages/_pytest/pathlib.py:540: in import_path
    spec.loader.exec_module(mod)  # type: ignore[union-attr]
/root/.local/share/hatch/env/virtual/pact-python/XZqP7kBn/pact-python/lib/python3.9/site-packages/_pytest/assertion/rewrite.py:178: in exec_module
    exec(co, module.__dict__)
examples/tests/test_01_provider_fastapi.py:28: in <module>
    async def mock_pact_provider_states(state: ProviderState) -> dict[str, str | None]:
/root/.local/share/hatch/env/virtual/pact-python/XZqP7kBn/pact-python/lib/python3.9/site-packages/fastapi/routing.py:704: in decorator
    self.add_api_route(
/root/.local/share/hatch/env/virtual/pact-python/XZqP7kBn/pact-python/lib/python3.9/site-packages/fastapi/routing.py:643: in add_api_route
    route = route_class(
/root/.local/share/hatch/env/virtual/pact-python/XZqP7kBn/pact-python/lib/python3.9/site-packages/fastapi/routing.py:403: in __init__
    return_annotation = get_typed_return_annotation(endpoint)
/root/.local/share/hatch/env/virtual/pact-python/XZqP7kBn/pact-python/lib/python3.9/site-packages/fastapi/dependencies/utils.py:238: in get_typed_return_annotation
    return get_typed_annotation(annotation, globalns)
/root/.local/share/hatch/env/virtual/pact-python/XZqP7kBn/pact-python/lib/python3.9/site-packages/fastapi/dependencies/utils.py:226: in get_typed_annotation
    annotation = evaluate_forwardref(annotation, globalns, globalns)
/root/.local/share/hatch/env/virtual/pact-python/XZqP7kBn/pact-python/lib/python3.9/site-packages/pydantic/_internal/_typing_extra.py:224: in eval_type_lenient
    return typing._eval_type(value, globalns, localns)  # type: ignore
/usr/local/lib/python3.9/typing.py:292: in _eval_type
    return t._evaluate(globalns, localns, recursive_guard)
/usr/local/lib/python3.9/typing.py:554: in _evaluate
    eval(self.__forward_code__, globalns, localns),
<string>:1: in <module>
    ???
E   TypeError: unsupported operand type(s) for |: 'type' and 'NoneType'
=========================== short test summary info ============================
ERROR examples/tests/test_01_provider_fastapi.py - TypeError: unsupported operand type(s) for |: 'type' and 'NoneType'
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
=============================== 1 error in 1.17s ===============================
Exit status: 2

3.10 and 3.11 failing because they need docker.

cirrus ci can support docker for linux workflows, macos can't though.

https://cirrus-ci.org/guide/docker-builder-vm/#under-the-hood

YOU54F · 2023-09-19T19:36:13Z

If I say as an end user, lift and shift an example, into my own repo (which for arguments sake is empty), how would I easily be able to work out what my deps look like for any particular example?

Could we solve that in a separate way, if the problem statement is, as a user, I easily want to replicate one of the python e2e examples as a bootstrap in a new repo or just on my local machine, could pact-python help me bootstrap that.

(or thinking wider, could our pact languages more generally do this, so its actually part of the core tool, maybe its a way of extensions, people can bring their own templates in, but we can use it for having our examples close to our working code in each respective language, having a core cli tool that can interrogate a repo and work out what example templates are available)

not something for this pr just food for thought

JP-Ellis · 2023-09-19T22:35:38Z

Wow thank you for all the comments!

docker not available on macos runners in cirrus ci or github actions.

licensing reasons on gha and because of apple virtulization restrictions for cirrus ci, which doesn't permit nested virtualization.

That's fine. I can disable these checks on Cirrus. As for GitHub Actions, I'm pretty sure I've used Docker images before or some other sort of containerisation/virtualisation (it's needed to build the wheels targetting arm64) 🤔

Personally, I'm fine with testing the examples against only a subset of the targets. The test suite should definitely be validated against all supported versions, but the examples are more pedagogical. The BooleanOptionalAction error is in fact because it was introduced in Python 3.9.

I also made changes in my PR to support switching between using a hosted OSS broker that PactFlow provides (for ci/testing/poking around)

59e2ef6

I was hitting rate limits as the CI tests would run against x python versions on 3 OS's, and 2 arches, whilst using fix app version and branches, and the same consumer/provider names.

We would be limited cos we are smashing the same endpoint with the same data xD

adding the commit and branch from the actual branch/commit of the repo along with an identifier for --<ci_system> would be useful to avoid clashing, rate limiting and identification, back, in case of discrepancies between any of the generated artefacts (pact files in the examples case)

Thanks! I did add the option in the examples allowing for --broker-url to be specified for this purpose. It's also useful if I'm running the broker locally and want to keep it running after the test, as opposed to having the test suite spinning up the container and shutting it down every time.

If I say as an end user, lift and shift an example, into my own repo (which for arguments sake is empty), how would I easily be able to work out what my deps look like for any particular example?
Could we solve that in a separate way, if the problem statement is, as a user, I easily want to replicate one of the python e2e examples as a bootstrap in a new repo or just on my local machine, could pact-python help me bootstrap that.
(or thinking wider, could our pact languages more generally do this, so its actually part of the core tool, maybe its a way of extensions, people can bring their own templates in, but we can use it for having our examples close to our working code in each respective language, having a core cli tool that can interrogate a repo and work out what example templates are available)

not something for this pr just food for thought

That's a good question, and the last commit (still a WIP) for this PR is going to check all of the documentation and make sure it is clear. This way, they can 'lift-and-shift' what is relevant to them.

I think from a pedagogical perspective, it is important to show the whole process at once, especially in a very simple example. If I, as a developer on a provider, just looked at the provider side, I don't think I would understand how Pact works at all. For this reason, I think it is important to have at least one complete end-to-end example.

It might be worth creating a separate pact-python-examples monorepo of examples (I note that other languages have such examples). These can then go into a lot more detail on the consumer side and provider side, and might be better suited to serve as a template.

JP-Ellis · 2023-09-20T03:46:02Z

This PR is now ready for review.

A note about merging this: It is built on top of #371 and therefore requires the latter to be merged before this one can be merged. I will be rebase this PR onto the new master branch.

Thanks @YOU54F for all of the comments so far!

With regards to the examples not running on Cirrus, I have opted to just disable them there. The test suite should detect regressions, unlike the example only serves to illustrate the use of pact-python. Let me know if that's alright with you.

For the purposes of showcasing an example, the previous `docker-compose` was rather excessive running the Pact Broker behind a Nginx proxy with a PostgreSQL backend. This simplifies the containers to just use the core Pact broker image and uses `sqlite` for the database. This commit also drops SSL/TLS from the example as it does not meaningfully contribute to the example. Signed-off-by: JP-Ellis <josh@jpellis.me>

This migrates the Pact FastAPI provider from the old standalone examples, and merges it with the new combined examples. The consumer tests are executed first which publishes the contracts with the broker. The provider test is then executed against the broker to verify compliance with the published contracts. This does, at this stage, create an unusual interdependence between tests which typically should be avoided. I plan to fix this at a later stage. Signed-off-by: JP-Ellis <josh@jpellis.me>

Following the changes to the FastAPI example, this migrates the Flask provider example to the new structure. The example relies on the consumer having published contracts, and the flask provider is verified against those contracts. Signed-off-by: JP-Ellis <josh@jpellis.me>

Update the README for the examples to match the new structure of the examples. Signed-off-by: JP-Ellis <josh@jpellis.me>

@YOU54F

Migrate the old isolated example and combine the test with the other examples so that they can all be run at once. Massive thanks to @YOU54F for identifying the switch up between the `given` and `expected`! Signed-off-by: JP-Ellis <josh@jpellis.me>

Try splitting the CI workflow to make use of services Signed-off-by: JP-Ellis <josh@jpellis.me>

JP-Ellis · 2023-09-22T03:17:31Z

In the latest changes:

Rebase the changes onto master now that the hatch PR has merged (as this PR uses hatch too)
Fixed an issue with the Message Pact example (thanks to @YOU54F for pinpointing the cause!)

This should be all good to merge 🥳

Initially, when having the tests completely separate, the examples were scoped to their own space and required setting `PYTHONPATH` to work. This is not needed if we change `import src.{mod}` to `import examples.src.{mod}`. Signed-off-by: JP-Ellis <josh@jpellis.me>

mefellows · 2023-09-26T06:46:35Z

The test suite should detect regressions, unlike the example only serves to illustrate the use of pact-python. Let me know if that's alright with you.

Philosophically, this makes sense to me. Especially as we look to introduce the BDD suite - making the examples be "regression" tests seems an abuse of them.

YOU54F · 2023-09-26T16:44:29Z

With regards to the examples not running on Cirrus, I have opted to just disable them there. The test suite should detect regressions, unlike the example only serves to illustrate the use of pact-python. Let me know if that's alright with you.

I think I would like to test them x-plat/x-arch but its not a deal-breaker for me on this PR (it was only recently introduced by myself) - assuming they aren't covering anything already covered by the existing test suite. (They almost serve at integration/regression tests for me of the main code base, which may mark its own homework with its unit test suite)

Philosophically, this makes sense to me. Especially as we look to introduce the BDD suite - making the examples be "regression" tests seems an abuse of them.

I don't see why they can't be both. If they aren't full examples that someone can copy and paste, and they aren't regression tests, what are they then? What purpose are they trying to solve? Living documentation? Then I'd prefer they were tested against all supported platforms.

I do think the BDD suite will be a great way of unifying examples across the different codebases, but I would see them both as serving as both examples and regression suites.

YOU54F · 2023-09-26T17:00:08Z

Hey hey,

Makefile consistency

Few comments around the Makefile, to help existing contributors, as it's in a bit of a limbo state post hatch.

make test doesn't run

hatch run test:all doesnt exist, needs to be hatch run test
Can this just run the unit tests

make examples doesn't run, neither do sub examples

Please update to support running hatch run example and sub examples, or remove from Makefile

make test:all or similar

Run unit tests and examples

make lint run the linter
make ci

runs linter, unit tests, coverage

.PHONY: test
test:
	hatch run all
	hatch run test:all
	coverage report -m --fail-under=100

Example testing coverage

Personally I would prefer to see the examples tested against all the supported versions of Python. Currently this PR proposes only testing against the latest which isn't sufficient in my opinion.

I think the code as proposed fails to run on 3.7/3.8 in the examples. This should be noted explicitly if we don't intend on testing this, as we are aware of it.

Dockerfile

I needed to add restart: always to my docker-compose.yml file otherwise it would consistently fail to start the broker

  broker:
    image: pactfoundation/pact-broker:latest-multi
    depends_on:
      - postgres
    ports:
      - "9292:9292"
    restart: always

Machine deets

🕙17:55:45 ❯ sw_vers
ProductName:            macOS
ProductVersion:         13.6
BuildVersion:           22G120
🕙17:58:59 ❯ uname -m
arm64
🕙17:59:05 ❯ python --version
Python 3.11.4
🕙17:59:10 ❯ docker --version           
Docker version 24.0.6, build ed223bc

YOU54F

Great work on the documenting in the examples, just reviewed all the comments and added some feedback / suggestions / typos.

Some comments above RE: the Makefile, would love just to have commands (make test/make example - out the box as part of this PR and we can make improvements down the line) so its not in a partially working state

examples/src/consumer.py

examples/tests/test_00_consumer.py

examples/tests/test_02_message_consumer.py

examples/tests/test_03_message_provider.py

Signed-off-by: JP-Ellis <josh@jpellis.me>

JP-Ellis · 2023-09-27T00:37:07Z

Thanks @YOU54F for the amazing review!

I've addressed the inline comments and have fixed the Makefile. I've also made sure the examples run on Python 3.8.

I still think that the e2e examples need not be run in the CI for all platforms, primarily because it relies on pulling and setting up a Docker image which slows things down a fair bit.

YOU54F

Awesome, thanks for the update, looks great

JP-Ellis self-assigned this Sep 15, 2023

JP-Ellis changed the title ~~Fix existing examples #370~~ Fix existing examples Sep 15, 2023

YOU54F self-requested a review September 15, 2023 14:43

YOU54F reviewed Sep 15, 2023

View reviewed changes

pyproject.toml Show resolved Hide resolved

JP-Ellis force-pushed the feat/examples branch 4 times, most recently from 80420b2 to be4f1ed Compare September 19, 2023 09:28

JP-Ellis force-pushed the feat/examples branch from be4f1ed to fa68610 Compare September 20, 2023 03:38

JP-Ellis marked this pull request as ready for review September 20, 2023 03:38

JP-Ellis force-pushed the feat/examples branch 13 times, most recently from cac28c0 to 8bb0bc2 Compare September 21, 2023 04:48

JP-Ellis added 3 commits September 21, 2023 15:01

chore(example): update readme

81ae787

Update the README for the examples to match the new structure of the examples. Signed-off-by: JP-Ellis <josh@jpellis.me>

JP-Ellis force-pushed the feat/examples branch 7 times, most recently from d3ceb98 to e6ba7a9 Compare September 22, 2023 03:02

JP-Ellis force-pushed the feat/examples branch from e6ba7a9 to 4d20c11 Compare September 22, 2023 03:13

chore(ci): split tests examples and lints

c8e0e7f

Try splitting the CI workflow to make use of services Signed-off-by: JP-Ellis <josh@jpellis.me>

JP-Ellis force-pushed the feat/examples branch from 4d20c11 to c8e0e7f Compare September 22, 2023 03:15

JP-Ellis enabled auto-merge (rebase) September 22, 2023 03:17

JP-Ellis disabled auto-merge September 22, 2023 03:30

JP-Ellis enabled auto-merge (rebase) September 22, 2023 03:30

mefellows self-requested a review September 26, 2023 06:48

YOU54F requested changes Sep 26, 2023

View reviewed changes

chore: address pr comments

848fda1

Signed-off-by: JP-Ellis <josh@jpellis.me>

JP-Ellis mentioned this pull request Sep 27, 2023

docs: Improve docs to have pact broker example #162

Closed

YOU54F approved these changes Sep 27, 2023

View reviewed changes

JP-Ellis merged commit 9896320 into master Sep 27, 2023

JP-Ellis deleted the feat/examples branch September 27, 2023 11:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix existing examples #387

Fix existing examples #387

JP-Ellis commented Sep 15, 2023

YOU54F commented Sep 19, 2023 •

edited

Loading

YOU54F commented Sep 19, 2023

YOU54F commented Sep 19, 2023 •

edited

Loading

YOU54F commented Sep 19, 2023

JP-Ellis commented Sep 19, 2023

JP-Ellis commented Sep 20, 2023

JP-Ellis commented Sep 22, 2023

mefellows commented Sep 26, 2023

YOU54F commented Sep 26, 2023

YOU54F commented Sep 26, 2023

YOU54F left a comment

JP-Ellis commented Sep 27, 2023

YOU54F left a comment

Fix existing examples #387

Fix existing examples #387

Conversation

JP-Ellis commented Sep 15, 2023

YOU54F commented Sep 19, 2023 • edited Loading

YOU54F commented Sep 19, 2023

YOU54F commented Sep 19, 2023 • edited Loading

YOU54F commented Sep 19, 2023

JP-Ellis commented Sep 19, 2023

JP-Ellis commented Sep 20, 2023

JP-Ellis commented Sep 22, 2023

mefellows commented Sep 26, 2023

YOU54F commented Sep 26, 2023

YOU54F commented Sep 26, 2023

Makefile consistency

Example testing coverage

Dockerfile

YOU54F left a comment

Choose a reason for hiding this comment

JP-Ellis commented Sep 27, 2023

YOU54F left a comment

Choose a reason for hiding this comment

YOU54F commented Sep 19, 2023 •

edited

Loading

YOU54F commented Sep 19, 2023 •

edited

Loading