-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test Framework Base #1
Conversation
cc @gbrail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you run rustfmt
and clippy
over the codebase?
$ rustup toolchain install stable --component clippy --component rustfmt
$ cargo clippy --target=wasm32-unknown-unknown --release --all-targets
$ cargo fmt
Signed-off-by: Christopher Agia <chrisagia@google.com>
Signed-off-by: Christopher Agia <chrisagia@google.com>
Signed-off-by: Christopher Agia <chrisagia@google.com>
Signed-off-by: Christopher Agia <chrisagia@google.com>
Signed-off-by: Christopher Agia <chrisagia@google.com>
Signed-off-by: Christopher Agia <chrisagia@google.com>
Signed-off-by: Christopher Agia <chrisagia@google.com>
Signed-off-by: Christopher Agia <chrisagia@google.com>
Signed-off-by: Christopher Agia <chrisagia@google.com>
Signed-off-by: Christopher Agia <chrisagia@google.com>
Signed-off-by: Christopher Agia <chrisagia@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, a few style comments.
src/expect_interface.rs
Outdated
|
||
/* proxy_log(), proxy_set_tick_period_millis(), proxy_replace_header_map_value(), proxy_remove_header_map_value() | ||
proxy_add_header_map_value(), proxy_send_loca_response(), etc. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you make them part of the same comment section, using the same style-prefix?
Signed-off-by: Christopher Agia <chrisagia@google.com>
Signed-off-by: Christopher Agia <chrisagia@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened a few issues to follow-up, but this looks great, thanks!
Signed-off-by: Christopher Agia <chrisagia@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Would it be possible to use Rust macros to avoid some of the repetition? You end up creating a lot of small objects the sole purpose of which is to handle the expecting -> returning idiom which would seem amenable to automation through macros.
.call_start() | ||
.execute_and_expect(ReturnType::None)?; | ||
|
||
let root_context = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: the VM contest which is used only for allocating memory is typically 1 whereas the first root context is typically 2. This isn't part of the ABI, but it is part of the Envoy host implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhm, is that a recent change? It used to be that the root context was always created with parent_context_id = 0
, and that's how some SDKs are inferring the type of the context to create (RootContext if parent_context_id == 0
, {Http,Stream}Context otherwise).
This pull request contains the base framework for the ABI test harness, enabling the user to set low-level and high-level expectations over a variety host-side functions and all functions exported by the proxy-wasm module. Remaining work includes the extension of this framework to support all host-side functions, and the associated low-level and high-level expectation settings.