-
Notifications
You must be signed in to change notification settings - Fork 328
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] fix TestLoadBlockchainfromDB #3521
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3521 +/- ##
==========================================
- Coverage 75.43% 74.56% -0.87%
==========================================
Files 247 252 +5
Lines 22845 23115 +270
==========================================
+ Hits 17233 17236 +3
- Misses 4685 4952 +267
Partials 927 927
Continue to review full report at Codecov.
|
tsfs, _ := classifyActions(blk.Actions) | ||
ms.counter += len(tsfs) | ||
ms.mu.Unlock() | ||
atomic.AddInt32(&ms.counter, int32(len(tsfs))) |
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.
good fix
blockchain/pubsubmanager.go
Outdated
ps.lock.RLock() | ||
defer ps.lock.RUnlock() | ||
blocklisteners := ps.blocklisteners | ||
for _, elem := range blocklisteners { |
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.
this change is not necessary?
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.
RLock
,RUnlock
is better here
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.
i mean, no need to define blocklisteners := ps.blocklisteners
, use like before
for _, elem := range ps.blocklisteners
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.
done
@@ -1142,6 +1137,8 @@ func TestLoadBlockchainfromDB(t *testing.T) { | |||
height := bc.TipHeight() | |||
fmt.Printf("Open blockchain pass, height = %d\n", height) | |||
require.NoError(addTestingTsfBlocks(cfg, bc, dao, ap)) | |||
//make sure pubsub is completed | |||
time.Sleep(time.Millisecond * 200) |
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.
use waitutil instead of sleep in the test
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.
done, thanks
@@ -1142,7 +1137,12 @@ func TestLoadBlockchainfromDB(t *testing.T) { | |||
height := bc.TipHeight() | |||
fmt.Printf("Open blockchain pass, height = %d\n", height) | |||
require.NoError(addTestingTsfBlocks(cfg, bc, dao, ap)) | |||
require.NoError(bc.Stop(ctx)) | |||
//make sure pubsub is completed | |||
err = testutil.WaitUntil(200*time.Millisecond, 3*time.Second, func() (bool, 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.
err = testutil.WaitUntil(200*time.Millisecond, 3*time.Second, func() (bool, error) {
return ms.Counter() == 24, nil
})
require.NoError(err)
require.NoError(bc.Stop(ctx))
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.
Your fix looks better
* upstream/master: (28 commits) [ioctl] Incorrect conversion between integer types (iotexproject#3522) [action] fix incorrect conversion between integer types (iotexproject#3545) [test] fix TestLoadBlockchainfromDB (iotexproject#3521) [ioctl] correct Chinese usage message (iotexproject#3510) fix err not hanled (iotexproject#3509) add ReadHeaderTimeout (iotexproject#3539) [iotcl] Reset config cmd (iotexproject#3496) Update gosec.yaml Update ci.yaml Update analysis.yaml Delete gosec.yaml.bak Create gosec.yaml [config] move config.ActPool to actpool package refactor (iotexproject#3514) Update ci.yaml Update analysis.yaml Update analysis.yaml [config] move config.Chain to blockchain package (iotexproject#3511) remove circleci (iotexproject#3498) [ioctl] Build block bucketlist command line into new ioctl (iotexproject#3469) [config] remove EVMNetworkID() and SetEVMNetworkID() (iotexproject#3503) ...
* upstream/master: (45 commits) Task: Get config cmd (iotexproject#3552) [ioctl] fix Errors unhandled (iotexproject#3567) fix dir permission and file inclusion (iotexproject#3566) [test] Disable workingset cache in the benchmark test (iotexproject#3558) [pkg] fix deferring unsafe method "Close" on type "*os.File" (iotexproject#3548) [action] Refactor handleTransfer() (iotexproject#3557) Add MinVersion in tls.Config (iotexproject#3562) [ioctl] Modify file permission as 0600 (iotexproject#3563) [httputil] add ReadHeaderTimeout (iotexproject#3550) [staking] unexport namespace (iotexproject#3551) move chanid metrics to chainservice (iotexproject#3544) [ioctl] fix log entries created from user input (iotexproject#3546) add log in rolldposctx (iotexproject#3553) fix uncontrolled data used in path expression (iotexproject#3547) [api] impl. TestGrpcServer_GetServerMeta (iotexproject#3559) [ioctl] Build action command line into new ioctl (iotexproject#3472) fix potential file inclusion via variable (iotexproject#3549) [ioctl] Incorrect conversion between integer types (iotexproject#3522) [action] fix incorrect conversion between integer types (iotexproject#3545) [test] fix TestLoadBlockchainfromDB (iotexproject#3521) ...
Description
Fixed
make test
sometimes failedFixes #3520
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist: