Skip to content

(parallel-executor): tests #2967

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
Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

(parallel-executor): tests #2967

wants to merge 19 commits into from

Conversation

AurelienFT
Copy link
Contributor

Linked Issues/PRs

Resolve #2951

Description

All tests needed to ensure the correct behavior of the parallel-executor

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog
  • New behavior is reflected in tests
  • The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

  • I have reviewed the code myself
  • I have created follow-up issues caused by this PR and linked them here

@AurelienFT AurelienFT added the no changelog Skip the CI check of the changelog modification label Apr 23, 2025
@AurelienFT AurelienFT self-assigned this Apr 23, 2025
@AurelienFT AurelienFT marked this pull request as ready for review April 23, 2025 11:45
@AurelienFT
Copy link
Contributor Author

@xgreenx I would like to have your feedback on the tests created here that should outline the behavior of parallel executor.

I'm still questioning myself about asking to the pool one time for each thread. For example in the first test scenario it means that only worker thread 1 will work and second never. Maybe if transaction source is informing when asking for example for thread 2 that there is nothing we can split the queue of thread 1 in the others.

I will start implementation on top of this PR.

netrome
netrome previously approved these changes Apr 24, 2025
View reviewed changes
Copy link
Contributor

@netrome netrome left a comment

Choose a reason for hiding this comment

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

Nice with the TDD approach! Some nits and questions from my side. I'm primarily curious about why we use threads and synchronous channels as opposed to tokio tasks and async channels.

Beyond that, I don't spot any obvious errors.

@AurelienFT AurelienFT requested a review from netrome April 24, 2025 09:21
@AurelienFT
Copy link
Contributor Author

@xgreenx in addition of my previous questions here : #2967 (comment)

I would like to ask you if you have an idea on how I can re-use the same DB as for the executor tests and avoid having a circular-dependency problem like this. The existing executor tests lives in fuel-core directly and so doesn't have the problem. Is it my only way to go also ?

segfault-magnet
segfault-magnet reviewed Apr 24, 2025
View reviewed changes
Copy link
Contributor

@segfault-magnet segfault-magnet left a comment

Choose a reason for hiding this comment

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

Unfortunately can only suggest readability nits at this point :)

@AurelienFT AurelienFT enabled auto-merge (squash) May 9, 2025 08:02
@AurelienFT AurelienFT changed the title Tests for parallel executor (parallel-executor): tests May 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rymnc rymnc rymnc approved these changes

@xgreenx xgreenx Awaiting requested review from xgreenx xgreenx is a code owner

@Dentosal Dentosal Awaiting requested review from Dentosal Dentosal is a code owner

@MitchTurner MitchTurner Awaiting requested review from MitchTurner MitchTurner is a code owner

@netrome netrome Awaiting requested review from netrome netrome is a code owner

@segfault-magnet segfault-magnet Awaiting requested review from segfault-magnet

At least 2 approving reviews are required to merge this pull request.

Assignees

@AurelienFT AurelienFT

Labels
no changelog Skip the CI check of the changelog modification
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create tests for the parallel-executor
4 participants
@AurelienFT @netrome @segfault-magnet @rymnc