Feature/add support and example messenger consumer using rust engine 380 #699
Conversation
…iterator, other small tweaks
|
Hey @valkolovos do you want me to kick off the gh workflows for some feedback? |
|
@YOU54F yes please! |
|
Hey 👋 I'm back from leave and I'll start looking at both PRs over the next couple of days. It's awesome to have new contributors, and thank you for contributing such a major contribution! As the PRs are quite large (a bit of 4k loc in all), I'll take my time reviewing them so that I can digest the changes, test things locally, and provide feedback. If it's alright, I might also add my own commits to the PRs for any minor fixes. Feel free to reach out now that I'm back in case you have any quick questions (on Slack, or in comments here on GitHub). |
There was a problem hiding this comment.
So I've had a first pass at this PR. First and foremost, amazing work!
The comments I've left above are mostly just questions to try and understand some of the decisions you've made. Please let me know what you think.
Before merging, there's a few relatively minor changes I would make. I'm more than happy to do these changes as you have already done the bulk of the work!
I can confirm everything is working great on Python 3.12, though there's a few minor changes required to ensure compatibility with Python 3.8 to 3.11 (mostly related to some typing-related stuff). For example, Self was not added to the typing module until recently, and the community package typing_extensions helps bridge the gap :) I'm more than happy to fix these.
Ensure that the Postgres image is fully up and running before launching the broker. This is unlikely to be an issue, but there's hardly any impact to adding this. Signed-off-by: JP-Ellis <josh@jpellis.me>
In some circumstances, the test will try and connect to the FastAPI/Flask server before the server has had a chance to be fully initialised. As these are very lightweight servers, a simple second wait after the process is spawned should suffice. Signed-off-by: JP-Ellis <josh@jpellis.me>
As the examples are meant to be pedagogical, the docstrings have been expanded to explain _why_ there is a Filesystem class which only raises `NotImplementedError`. Signed-off-by: JP-Ellis <josh@jpellis.me>
Signed-off-by: JP-Ellis <josh@jpellis.me>
There were a few minor issues with the typing annotations: - `Callable` takes two arguments: 1. A list of types for the arguments of the function 2. A single type for the function's return - Prefer the use of the (more succinct) `|` instead of `Union[...]` Signed-off-by: JP-Ellis <josh@jpellis.me>
Signed-off-by: JP-Ellis <josh@jpellis.me>
As the result is from a single asynchronous message interaction, it seemed like a more appropriate name. As part of the refactor, the declaration of the class has been moved and some minor refactoring took place too. Signed-off-by: JP-Ellis <josh@jpellis.me>
From Pact V4, it is possible for a single Pact to mix different interaction types; that is, to combine sync/async messages, and HTTP interactions. As such, I think it is best to keep a single `Pact` class. Signed-off-by: JP-Ellis <josh@jpellis.me>
Signed-off-by: JP-Ellis <josh@jpellis.me>
Especially in light of the addition of asynchronous messages which only have one part. Signed-off-by: JP-Ellis <josh@jpellis.me>
The safety concern is handled by the use of `OwnedString`. Signed-off-by: JP-Ellis <josh@jpellis.me>
Signed-off-by: JP-Ellis <josh@jpellis.me>
Signed-off-by: JP-Ellis <josh@jpellis.me>
Signed-off-by: JP-Ellis <josh@jpellis.me>
Large commit which implements quite a large number of FFI functions. Signed-off-by: JP-Ellis <josh@jpellis.me>
Signed-off-by: JP-Ellis <josh@jpellis.me>
The `interactions` method provides the necessary functionality Signed-off-by: JP-Ellis <josh@jpellis.me>
Signed-off-by: JP-Ellis <josh@jpellis.me>
The provider states don't make sense for pacts, as they are associated with the individual interactions, as opposed to the pact itself. Signed-off-by: JP-Ellis <josh@jpellis.me>
Signed-off-by: JP-Ellis <josh@jpellis.me>
Objects generated by the FFI may sometimes be owned by another object, in which case deleting them when they are out of scope in Python is invalid. The instantiators for these classes have been adjusted to take an optional `owned` keyword argument. If `owned` is `True`, then the `__del__` function for the class will do nothing. Signed-off-by: JP-Ellis <josh@jpellis.me>
Signed-off-by: JP-Ellis <josh@jpellis.me>
The use of `yield from` may sometimes result in the parent instance being dropped, which then invalidates the underlying iterator. Adding a final `return` statement (even if it does nothing) ensures that Python finishes the `yield from` statement first before the function is finished. Signed-off-by: JP-Ellis <josh@jpellis.me>
The former `get_contents` FFI return the raw body of the interaction, including matching rules and generators, which aren't very useful when verifying a message consume. The new methods processes the payload, replacing the matching rules and generators with values as would be actually expected by the consumer. Signed-off-by: JP-Ellis <josh@jpellis.me>
Signed-off-by: JP-Ellis <josh@jpellis.me>
Signed-off-by: JP-Ellis <josh@jpellis.me>
The exceptions are to be returned during verification. Signed-off-by: JP-Ellis <josh@jpellis.me>
Signed-off-by: JP-Ellis <josh@jpellis.me>
The named tuple provides an improved experience for developers. Signed-off-by: JP-Ellis <josh@jpellis.me>
With the other changes in the FFI and Pact Python library, a significant refactor of the tests was introduced. A few steps were combined, though by and large the functionality remains the same. Signed-off-by: JP-Ellis <josh@jpellis.me>
Signed-off-by: JP-Ellis <josh@jpellis.me>
Signed-off-by: JP-Ellis <josh@jpellis.me>
Signed-off-by: JP-Ellis <josh@jpellis.me>
Now that the FFI library supports Windows ARM, we can add it to the list. Also a minor update to make use of the ARM runners when building the macOS ARM wheel. Signed-off-by: JP-Ellis <josh@jpellis.me>
Signed-off-by: JP-Ellis <josh@jpellis.me>
Signed-off-by: JP-Ellis <josh@jpellis.me>
| def assert_type(expected_type: str, value: Any) -> None: # noqa: ANN401 | ||
| logger.debug("Ensuring that %s is of type %s", value, expected_type) | ||
| if expected_type == "integer": | ||
| assert value is not None | ||
| assert isinstance(value, int) or re.match(r"^\d+$", value) | ||
| else: | ||
| msg = f"Unknown type: {expected_type}" | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
since this only asserts whether or not the value is an integer, should we rename it to assert_is_integer or something like that?
There was a problem hiding this comment.
I did consider that, but I figured there might be a need to check for other types in other parts of the compatibility suite; in which case I would like to move the declaration of this function out into the util module 🙂
But I won't do that prematurely...
There was a problem hiding this comment.
How about we change it now and, should there be a need to check for other types later, it can be refactored?
Signed-off-by: JP-Ellis <josh@jpellis.me>
Signed-off-by: JP-Ellis <josh@jpellis.me>
|
Closing this as it has been supplanted by #714 |
valkolovos
deleted the
feature/add-support-and-example-messenger-consumer-using-rust-engine-380
branch
📝 Summary
Feature/add support and example messenger consumer using rust engine
🔥 Motivation
Work for Add support and example for a message consumer test using Rust engine #380
🔨 Test Plan
Implemented tests for the compatibility suite. Specifically message consumer tests for v3 and v4.