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.

Sign up for GitHub

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

Jump to bottom

fix: TestTxPool_ConcurrentlyAddingTx #1501

Merged
merged 1 commit into from
Sep 25, 2024
Merged

fix: TestTxPool_ConcurrentlyAddingTx #1501

merged 1 commit into from
Sep 25, 2024

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Sep 25, 2024

Closes #1383

Testing

$ go test ./... -run "TestTxPool_ConcurrentlyAddingTx" -count=1000
ok  	github.com/tendermint/tendermint/mempool/cat	0.402s

@rootulp rootulp self-assigned this Sep 25, 2024
@rootulp rootulp requested a review from a team as a code owner September 25, 2024 14:32
@rootulp rootulp requested review from cmwaters, rach-id, evan-forbes and ninabarbakadze and removed request for a team September 25, 2024 14:32
@rootulp rootulp enabled auto-merge (squash) September 25, 2024 17:21
@rootulp rootulp mentioned this pull request Sep 25, 2024
Merged
2 tasks
@rootulp rootulp merged commit 9327f2b into celestiaorg:v0.34.x-celestia Sep 25, 2024
16 of 18 checks passed
staheri14
staheri14 reviewed Sep 25, 2024
View reviewed changes
mempool/cat/pool_test.go
@@ -747,7 +748,10 @@ func TestTxPool_ConcurrentlyAddingTx(t *testing.T) {
errCount++
}
}
require.Equal(t, numTxs-1, errCount)
// At least one tx should succeed and get added to the mempool without an error.
Copy link
Contributor

Choose a reason for hiding this comment

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

At least one tx should succeed and get added to the mempool without an error.

I've already approved it, but I was thinking we should maybe set it to 2, or something higher than 1, to ensure the concurrent adding works properly.

rootulp added a commit that referenced this pull request Sep 30, 2024
@rootulp @mergify
@sergio-mena @dependabot @jmalicevic @lasarojc @thanethomson @mmsqe @yihuang @stevenferrer @chillyvee @alijnmerchant21 @p-offtermatt @cason @hvanz @adizere @ebuchman @andynog @melekes @troykessler @forcodedancing @mzabaluev @AlexsandroRyan @alesforz @faddat
chore: pull cometbft v0.34.35 (#1495)
65c778d 
Closes #1468 by
pulling upstream
[v0.34.35](https://github.com/cometbft/cometbft/releases/tag/v0.34.35)
with these notable merge conflicts:
- Our repo upgraded Go past what upstream has so I choose to retain the
more recent Go version

And these changes to make unit tests pass:
1. Refactor CI to run all tests in one execution. It takes the same time
([4
mins](https://github.com/celestiaorg/celestia-core/actions/runs/11038290102/job/30661243408?pr=1495))
as it [previously
did](https://github.com/celestiaorg/celestia-core/actions/runs/10956425121).
But now it's easier to reason about the flakes.
2. Included #1503
3. Included #1501

## FLUPs
1. #1504
2. Opens #1502 and
skips that test

## Testing

I used go mod replace and verified that celestia-app can use this
version of celestia-core.
- [x] `single-node.sh`
- [x] `mocha.sh`

---------

Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: Thane Thomson <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Sergio Mena <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Jasmina Malicevic <[email protected]>
Co-authored-by: Lasaro <[email protected]>
Co-authored-by: Thane Thomson <[email protected]>
Co-authored-by: mmsqe <[email protected]>
Co-authored-by: yihuang <[email protected]>
Co-authored-by: Steven Ferrer <[email protected]>
Co-authored-by: Chill Validation <[email protected]>
Co-authored-by: Aliasgar Merchant <[email protected]>
Co-authored-by: Philip Offtermatt <[email protected]>
Co-authored-by: Daniel <[email protected]>
Co-authored-by: Hernán Vanzetto <[email protected]>
Co-authored-by: Adi Seredinschi <[email protected]>
Co-authored-by: Ethan Buchman <[email protected]>
Co-authored-by: Andy Nogueira <[email protected]>
Co-authored-by: Anton Kaliaev <[email protected]>
Co-authored-by: Troy Kessler <[email protected]>
Co-authored-by: forcodedancing <[email protected]>
Co-authored-by: Mikhail Zabaluev <[email protected]>
Co-authored-by: Alexsandro <[email protected]>
Co-authored-by: Alessandro <[email protected]>
Co-authored-by: Jacob Gadikian <[email protected]>
rach-id pushed a commit that referenced this pull request Nov 18, 2024
@rootulp @rach-id
fix: TestTxPool_ConcurrentlyAddingTx (#1501)
f04bc28 
Closes #1383

## Testing

```
$ go test ./... -run "TestTxPool_ConcurrentlyAddingTx" -count=1000
ok  	github.com/tendermint/tendermint/mempool/cat	0.402s
```
rach-id pushed a commit that referenced this pull request Nov 18, 2024
@rootulp @mergify
@sergio-mena @dependabot @jmalicevic @lasarojc @thanethomson @mmsqe @yihuang @stevenferrer @chillyvee @alijnmerchant21 @p-offtermatt @cason @hvanz @adizere @ebuchman @andynog @melekes @troykessler @forcodedancing @mzabaluev @AlexsandroRyan @alesforz @faddat @rach-id
chore: pull cometbft v0.34.35 (#1495)
e341292 
Closes #1468 by
pulling upstream
[v0.34.35](https://github.com/cometbft/cometbft/releases/tag/v0.34.35)
with these notable merge conflicts:
- Our repo upgraded Go past what upstream has so I choose to retain the
more recent Go version

And these changes to make unit tests pass:
1. Refactor CI to run all tests in one execution. It takes the same time
([4
mins](https://github.com/celestiaorg/celestia-core/actions/runs/11038290102/job/30661243408?pr=1495))
as it [previously
did](https://github.com/celestiaorg/celestia-core/actions/runs/10956425121).
But now it's easier to reason about the flakes.
2. Included #1503
3. Included #1501

## FLUPs
1. #1504
2. Opens #1502 and
skips that test

## Testing

I used go mod replace and verified that celestia-app can use this
version of celestia-core.
- [x] `single-node.sh`
- [x] `mocha.sh`

---------

Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: Thane Thomson <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Sergio Mena <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Jasmina Malicevic <[email protected]>
Co-authored-by: Lasaro <[email protected]>
Co-authored-by: Thane Thomson <[email protected]>
Co-authored-by: mmsqe <[email protected]>
Co-authored-by: yihuang <[email protected]>
Co-authored-by: Steven Ferrer <[email protected]>
Co-authored-by: Chill Validation <[email protected]>
Co-authored-by: Aliasgar Merchant <[email protected]>
Co-authored-by: Philip Offtermatt <[email protected]>
Co-authored-by: Daniel <[email protected]>
Co-authored-by: Hernán Vanzetto <[email protected]>
Co-authored-by: Adi Seredinschi <[email protected]>
Co-authored-by: Ethan Buchman <[email protected]>
Co-authored-by: Andy Nogueira <[email protected]>
Co-authored-by: Anton Kaliaev <[email protected]>
Co-authored-by: Troy Kessler <[email protected]>
Co-authored-by: forcodedancing <[email protected]>
Co-authored-by: Mikhail Zabaluev <[email protected]>
Co-authored-by: Alexsandro <[email protected]>
Co-authored-by: Alessandro <[email protected]>
Co-authored-by: Jacob Gadikian <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@staheri14 staheri14 staheri14 approved these changes

@evan-forbes evan-forbes evan-forbes approved these changes

@cmwaters cmwaters Awaiting requested review from cmwaters cmwaters is a code owner automatically assigned from celestiaorg/celestia-core

@rach-id rach-id Awaiting requested review from rach-id rach-id is a code owner automatically assigned from celestiaorg/celestia-core

@ninabarbakadze ninabarbakadze Awaiting requested review from ninabarbakadze

Assignees

@rootulp rootulp

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TestTxPool_ConcurrentlyAddingTx is flaky
3 participants
@rootulp @staheri14 @evan-forbes