-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
[GraphQL] fix issue with failed dry run and add more e2e test #16248
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
results | ||
.into_iter() | ||
.map(DryRunEffect::try_from) | ||
.collect::<Result<Vec<_>, Error>>()?, |
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.
Will this return an error in case DryRun fails?
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.
When a dry run fails, dev_inspect_results.results
would be None
and dev_inspect_results.error
would be Some
. If the dry run succeeds, then the reverse is true. Here we are in the branch where the dry run succeeds and results
is Some
. And to clarify, the Error
in collect::<Result<Vec<_>, Error>>()?
is potential internal errors from DryRunEffect::try_from
.
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.
Nice, thanks @emmazzz !
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.
Nice that we do get error results back from dev inspect/ dry run as well
80ff458
to
557f134
Compare
## Description Currently we assume that the dev inspect result coming from fullnode is always `Some` but it can actually be `None` if the transaction failed to execute. This PR fixes that and adds a few more e2e test cases for this scenario as well as a scenario where a `TransactionKind` is provided. ## Test Plan Added tests --- If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes
## Description Currently we assume that the dev inspect result coming from fullnode is always `Some` but it can actually be `None` if the transaction failed to execute. This PR fixes that and adds a few more e2e test cases for this scenario as well as a scenario where a `TransactionKind` is provided. ## Test Plan Added tests --- If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes
Description
Currently we assume that the dev inspect result coming from fullnode is always
Some
but it can actually beNone
if the transaction failed to execute. This PR fixes that and adds a few more e2e test cases for this scenario as well as a scenario where aTransactionKind
is provided.Test Plan
Added tests
If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section.
Type of Change (Check all that apply)
Release notes