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

[json-rpc] add more tests #6280

Merged
merged 5 commits into from
Sep 30, 2020
Merged

[json-rpc] add more tests #6280

merged 5 commits into from
Sep 30, 2020

Conversation

xli
Copy link
Contributor

@xli xli commented Sep 30, 2020

  1. added more corner cases test cases
  2. added tests for all different types event data views
  3. removed a wrapper when parsing event, the original version returns "unknown" event if parsing failed, which hides real error, changed to return server internal error to expose problem.

Still missing EventDataView::UpgradeEvent test, don't know how to generate it, will need talk to @runtian-zhou about it and add test later.

New event CreateAccountEvent is added, it is created recently (#6156).
Added new test to ensure there is no unknown events in integration tests at the end, to make test pass, created blank CreateAccountEvent for now (tried to decoded it, but got an error, so try to keep this diff simpler, will talk to @sblackshear to add created account address field if necessary)

@libra-action
Copy link

This PR may have modified JSON-RPC server code.

Breaking changes policy:

  1. Chaning JSON-RPC method signatures or removing/modifying fields
    in the response in /V1 will not be accepted.
  2. Adding fields is ok.
  3. Adding a new JSON-RPC method is ok.

Please ensure you have also done the following to update the docs:

  1. Update json-rpc/API-CHANGELOG.md.
  2. Add/update type/method documentation under json-rpc/docs.

@xli xli requested a review from zekun000 September 30, 2020 19:28
Copy link
Contributor

@ma2bd ma2bd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

key: BytesView::from(event.key().as_bytes()),
sequence_number: event.sequence_number(),
transaction_version: txn_version,
data: event_data.unwrap_or(EventDataView::Unknown {}),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed

@xli
Copy link
Contributor Author

xli commented Sep 30, 2020

@bors-libra land

@github-actions
Copy link

Cluster Test Result

Compatibility test results for land_81693505 ==> land_835774f6 (PR)
1. All instances running land_81693505, generating some traffic on network
2. First validator land_81693505 ==> land_835774f6, to validate storage
3. First batch validators (14) land_81693505 ==> land_835774f6, to test consensus
4. Second batch validators (15) land_81693505 ==> land_835774f6, to upgrade rest of the validators
5. All full nodes (30) land_81693505 ==> land_835774f6, to finish the network upgrade
all up : 1005 TPS, 4506 ms latency, 5500 ms p99 latency, no expired txns
Logs: http://kibana.ct-2-k8s-testnet.aws.hlw3truzy4ls.com/app/kibana#/discover?_g=(time:(from:'2020-09-30T20:00:57Z',to:'2020-09-30T20:21:48Z'))
Dashboard: http://grafana.ct-2-k8s-testnet.aws.hlw3truzy4ls.com/d/2XqUIhnWz/performance?from=1601496057000&to=1601497308000
Validator 1 logs: http://kibana.ct-2-k8s-testnet.aws.hlw3truzy4ls.com/app/kibana#/discover?_g=(time:(from:'2020-09-30T20:00:57Z',to:'2020-09-30T20:21:48Z'))&_a=(columns:!(log),query:(language:kuery,query:'kubernetes.pod_name:"val-1"'),sort:!(!('@timestamp',desc)))

Repro cmd:

./scripts/cti --tag land_81693505 --cluster-test-tag land_835774f6 -E RUST_LOG=debug -E BATCH_SIZE=15 -E UPDATE_TO_TAG=land_835774f6 --report report.json --suite land_blocking_compat

🎉 Land-blocking cluster test passed! 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants