Replace #[should_panic] with assertions in HTTP example tests#3705
Merged
jvff merged 14 commits intolinera-io:mainfrom Apr 3, 2025
Merged
Conversation
Return a `Result` instead of panicking, so that the user can match the error if needed.
Return a `Result` instead of panicking, so that the user can match the error if needed.
Ensure application integration tests can match against Wasm execution errors.
Ensure application integration tests can match against execution errors.
Ensure application integration tests can match against execution errors.
Ensure application integration tests can match against execution errors.
jvff
force-pushed
the
replace-should-panics-with-assertions-in-http-tests
branch
from
42ecafd to
9d90a53
Compare
ma2bd
approved these changes
Apr 2, 2025
linera-chain/src/lib.rs
Outdated
| } | ||
|
|
||
| #[derive(Copy, Clone, Debug)] | ||
| #[derive(Copy, Clone, Debug, Eq, PartialEq)] |
Contributor
There was a problem hiding this comment.
#[cfg_attr(with_testing, derive(Eq, PartialEq))] ?
Make it simple to compare two instances of the type.
Centralize the boilerplate code to extract an `ExecutionError` from a `WorkerError`.
Centralize the boilerplate code to extract an `ExecutionError` from a `TryGraphQLQueryError`.
Be more precise with the expected failure.
Allow users to match the error for more precise test assertions.
Return a `Result` instead of panicking, so that the user can match the error if needed.
Allow extracting an `ExecutionError` that occurred while proposing a block after a GraphQL mutation.
Be more precise with the expected failure.
jvff
force-pushed
the
replace-should-panics-with-assertions-in-http-tests
branch
from
9d90a53 to
a76e0a3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The integration tests for the HTTP Requests How-To example (#3509) used the
#[should_panic]attribute for testing failures. However, as pointed out during review, these are quite broad and may lead to mistakes. Being able to assert the exact error leads to better tests and a better example for external developers.Proposal
Add
try_query,try_graphql_queryandtry_graphql_mutationhelper methods to theActiveChaintest interface. Use the new methods to replace usages of#[should_panic]with error type assertions.To reduce boilerplate code, a
WorkerError::expect_execution_errortest helper method was added, that dives into the internalExecutionErrorstored inside aWorkerError.Test Plan
CI should catch any regressions caused by this refactor to the tests.
Release Plan
Links