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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
Signed-off-by: JP-Ellis <[email protected]>
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 <[email protected]>
Signed-off-by: JP-Ellis <[email protected]>
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 <[email protected]>
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 <[email protected]>
Signed-off-by: JP-Ellis <[email protected]>
Especially in light of the addition of asynchronous messages which only have one part. Signed-off-by: JP-Ellis <[email protected]>
The safety concern is handled by the use of `OwnedString`. Signed-off-by: JP-Ellis <[email protected]>
Signed-off-by: JP-Ellis <[email protected]>
Signed-off-by: JP-Ellis <[email protected]>
Signed-off-by: JP-Ellis <[email protected]>
Large commit which implements quite a large number of FFI functions. Signed-off-by: JP-Ellis <[email protected]>
Signed-off-by: JP-Ellis <[email protected]>
The `interactions` method provides the necessary functionality Signed-off-by: JP-Ellis <[email protected]>
Signed-off-by: JP-Ellis <[email protected]>
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 <[email protected]>
Signed-off-by: JP-Ellis <[email protected]>
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 <[email protected]>
Signed-off-by: JP-Ellis <[email protected]>
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 <[email protected]>
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 <[email protected]>
Signed-off-by: JP-Ellis <[email protected]>
Signed-off-by: JP-Ellis <[email protected]>
The exceptions are to be returned during verification. Signed-off-by: JP-Ellis <[email protected]>
Signed-off-by: JP-Ellis <[email protected]>
The named tuple provides an improved experience for developers. Signed-off-by: JP-Ellis <[email protected]>
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 <[email protected]>
Signed-off-by: JP-Ellis <[email protected]>
Signed-off-by: JP-Ellis <[email protected]>
Signed-off-by: JP-Ellis <[email protected]>
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 <[email protected]>
Signed-off-by: JP-Ellis <[email protected]>
Signed-off-by: JP-Ellis <[email protected]>
| 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 <[email protected]>
Signed-off-by: JP-Ellis <[email protected]>
|
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.