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: baseapp: add concurrent test for querying GRPCRouter #10371

Merged
merged 1 commit into from
Oct 16, 2021
Merged

test: baseapp: add concurrent test for querying GRPCRouter #10371

merged 1 commit into from
Oct 16, 2021

Conversation

odeke-em
Copy link
Collaborator

@odeke-em odeke-em commented Oct 14, 2021

Runs 1,000 concurrent requests for baseapp.GRPCQueryRouter,
this test requires that we enable: go test -race.

Closes #10324

@amaury1093 amaury1093 changed the title test! baseapp: add concurrent test for querying GRPCRouter test: baseapp: add concurrent test for querying GRPCRouter Oct 15, 2021
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

OK to merge this, but I think it could be improved too.

baseapp/grpcrouter_test.go Outdated Show resolved Hide resolved
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

LGTM. agree with @AmauryM

Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

If possible let's add one more query worker in this PR - this way we close the issue.

baseapp/grpcrouter_test.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 16, 2021

Codecov Report

Merging #10371 (de01d92) into master (717dcdf) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head de01d92 differs from pull request most recent head 5d899e8. Consider uploading reports for the commit 5d899e8 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10371      +/-   ##
==========================================
- Coverage   64.27%   64.27%   -0.01%     
==========================================
  Files         572      572              
  Lines       54230    54230              
==========================================
- Hits        34859    34858       -1     
- Misses      17389    17390       +1     
  Partials     1982     1982              
Impacted Files Coverage Δ
crypto/keys/internal/ecdsa/privkey.go 82.45% <0.00%> (-1.76%) ⬇️

@tac0turtle tac0turtle added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 16, 2021
Runs 1,000 concurrent requests for baseapp.GRPCQueryRouter,
this test requires that we enable: go test -race.
Runs 2 different scenarios for the same handler:
* same client connection being used concurrently
* unique client connections

Fixes #10324
@mergify mergify bot merged commit c0cc052 into cosmos:master Oct 16, 2021
@odeke-em odeke-em deleted the baseapp-add-grpcRoute-concurrent-test-for-datarace branch October 16, 2021 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. T: Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add load tests to verify no race condition exists in app gRPC queries
4 participants