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: add QGB queries test #1374

Merged
merged 3 commits into from
Feb 13, 2023
Merged

Conversation

rach-id
Copy link
Member

@rach-id rach-id commented Feb 13, 2023

Overview

Closes #1343

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@rach-id rach-id added x/qgb testing items that are strictly related to adding or extending test coverage labels Feb 13, 2023
@rach-id rach-id self-assigned this Feb 13, 2023
@MSevey MSevey requested a review from a team February 13, 2023 13:32
Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

LGTM with one small test name fix

app/test/qgb_rpc_test.go Outdated Show resolved Hide resolved
Co-authored-by: Rootul P <rootulp@gmail.com>
@MSevey MSevey requested a review from a team February 13, 2023 14:55
@codecov-commenter
Copy link

Codecov Report

Merging #1374 (56c2645) into main (ff69444) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1374   +/-   ##
=======================================
  Coverage   48.35%   48.35%           
=======================================
  Files          79       79           
  Lines        4438     4438           
=======================================
  Hits         2146     2146           
  Misses       2113     2113           
  Partials      179      179           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Comment on lines +18 to +19
_, cctx := testnode.DefaultNetwork(t, time.Millisecond)
h, err := cctx.WaitForHeightWithTimeout(401, time.Minute)
Copy link
Member

Choose a reason for hiding this comment

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

[definitely optional]
if we don't want to wait a minute here, we can change the genesis state to include a significantly shorter data window param

Copy link
Member Author

Choose a reason for hiding this comment

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

We won't wait a minute. It's just a worst case scenario. Generally, with a block time of 1ms, 401 blocks will be created in less than half a second.
Yes, we can do that, but ,with fast blocktime, i dont see why tinker with the genesis state and do more changes.

Im okey with any tbh, whichever you prefer 🐱

Copy link
Member

Choose a reason for hiding this comment

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

its fine as is, just an optimization 👍

@rach-id rach-id merged commit d44cc0c into celestiaorg:main Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing items that are strictly related to adding or extending test coverage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add LastValsetBeforeNonce and other QGB RPC methods tests
4 participants