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(ethexe): Introduce two-step Service run #4311

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

techraed
Copy link
Member

@techraed techraed commented Oct 24, 2024

Resolves #4099

TODO:

  • use macro to avoid copy-paste in EventsStreamProducer

@techraed techraed added the A1-inprogress Issue is in progress or PR draft is not ready to be reviewed label Oct 24, 2024
@techraed techraed self-assigned this Oct 24, 2024
@techraed techraed added A0-pleasereview PR is ready to be reviewed by the team and removed A1-inprogress Issue is in progress or PR draft is not ready to be reviewed labels Oct 28, 2024
@breathx breathx added A1-inprogress Issue is in progress or PR draft is not ready to be reviewed A3-gotissues PR occurred to have issues after the review and removed A0-pleasereview PR is ready to be reviewed by the team labels Nov 5, 2024
@techraed techraed added A0-pleasereview PR is ready to be reviewed by the team and removed A1-inprogress Issue is in progress or PR draft is not ready to be reviewed A3-gotissues PR occurred to have issues after the review labels Nov 12, 2024
@techraed
Copy link
Member Author

I'd say that tests are really fragile: removing timers results in underlying spawned tasks being not synchronized, which creates many obstacles in testing. The approach to test impl should be changed in future, so we can write them easily without thinking too much about underlying details of how Service and other entities are implemented.

6e450c4 - this works perfectly fine, but requires some timeout for sync.

a465d72 - this avoids sync problems when channgels try to read data, when there's no actual data sent from the anvil, because it works in a non-block-production mode. However, it requires increasing a number of attempts to get the receipt for the sent tx (could be the anvil problem).

let events_stream_producer = observer.events_stream_producer();

self.run_inner(events_stream_producer).await.map_err(|err| {
log::error!("Service finished work with error: {:?}", err);

Choose a reason for hiding this comment

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

Suggested change
log::error!("Service finished work with error: {:?}", err);
log::error!("Service finished work with error: {err:?}");

@@ -297,7 +297,9 @@ impl<T: Transport + Clone, N: Network> TryGetReceipt<T, N> for PendingTransactio
Err(err) => err,
};

for _ in 0..3 {
log::trace!("Failed to get transaction receipt for {tx_hash}. Retrying...");
for n in 0..25 {

Choose a reason for hiding this comment

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

it's ok for tests, but bad for production

rpc,
block_time,
signer: _signer,

Choose a reason for hiding this comment

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

Suggested change
signer: _signer,

.run_inner(events_stream_producer)
.await
.map_err(|err| {
log::error!("Service finished work with error: {:?}", err);

Choose a reason for hiding this comment

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

Suggested change
log::error!("Service finished work with error: {:?}", err);
log::error!("Service finished work with error: {err:?}");

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-pleasereview PR is ready to be reviewed by the team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ethexe: append possibility to receive info that service is started to listen blocks
3 participants