Skip to content
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

Merged
merged 22 commits into from
Aug 12, 2020
Merged

Test Framework Base #1

merged 22 commits into from
Aug 12, 2020

Conversation

agiachris
Copy link
Contributor

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.

@PiotrSikora
Copy link
Member

cc @gbrail

Copy link
Member

@PiotrSikora PiotrSikora left a 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

Cargo.toml Outdated Show resolved Hide resolved
Signed-off-by: Christopher Agia <chrisagia@google.com>
Signed-off-by: Christopher Agia <chrisagia@google.com>
src/types.rs Outdated Show resolved Hide resolved
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>
examples/hello_world.rs Outdated Show resolved Hide resolved
examples/http_auth_random.rs Outdated Show resolved Hide resolved
examples/http_headers.rs Outdated Show resolved Hide resolved
examples/http_auth_random.rs Outdated Show resolved Hide resolved
examples/http_auth_random.rs Outdated Show resolved Hide resolved
src/expectations.rs Outdated Show resolved Hide resolved
src/host_settings.rs Outdated Show resolved Hide resolved
src/host_settings.rs Outdated Show resolved Hide resolved
src/hostcalls.rs Outdated Show resolved Hide resolved
src/hostcalls.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
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>
Cargo.toml Outdated Show resolved Hide resolved
Signed-off-by: Christopher Agia <chrisagia@google.com>
Signed-off-by: Christopher Agia <chrisagia@google.com>
Signed-off-by: Christopher Agia <chrisagia@google.com>
Copy link
Member

@PiotrSikora PiotrSikora left a 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/host_settings.rs Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved

/* 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.
*/
Copy link
Member

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?

examples/hello_world.rs Outdated Show resolved Hide resolved
examples/http_auth_random.rs Outdated Show resolved Hide resolved
examples/http_auth_random.rs Outdated Show resolved Hide resolved
examples/http_headers.rs Outdated Show resolved Hide resolved
examples/http_headers.rs Outdated Show resolved Hide resolved
Signed-off-by: Christopher Agia <chrisagia@google.com>
Signed-off-by: Christopher Agia <chrisagia@google.com>
Copy link
Member

@PiotrSikora PiotrSikora left a 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!

Cargo.toml Outdated Show resolved Hide resolved
Signed-off-by: Christopher Agia <chrisagia@google.com>
Copy link

@jplevyak jplevyak left a 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;

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.

Copy link
Member

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).

@PiotrSikora PiotrSikora merged commit 51ac990 into proxy-wasm:master Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants