-
Notifications
You must be signed in to change notification settings - Fork 276
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
[refactor] #4039: Enhance ClientCli Wait Mechanism for Transaction Completion in pytests and Remove Fixed Sleeps #4340
Conversation
Pull Request Test Coverage Report for Build 8555785955Details
💛 - Coveralls |
self.threads = [] | ||
for port in peers_ports: | ||
self.config.update_torii_url(port) | ||
thread = threading.Thread(target=self.listen_to_events, args=(port,)) |
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.
listen_to_events
expects config_path
but you are passing port
here, looks suspicious
output = process.stdout.readline() | ||
if not output: | ||
break | ||
with self.event_data_lock: |
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 don't think you need a lock here, python dict
is thread-safe due to GIL.
if config_path in self.event_data: | ||
self.event_data[config_path] += output | ||
else: | ||
self.event_data[config_path] = output |
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.
Instead of default dict you can use deafultdict(str)
here, this way code could be simplified:
if config_path in self.event_data: | |
self.event_data[config_path] += output | |
else: | |
self.event_data[config_path] = output | |
self.event_data[config_path] += output |
self.transaction_status = {} | ||
self.threads = [] | ||
for port in peers_ports: | ||
self.config.update_torii_url(port) |
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 part looks like every thread will see only latest call to update_torii_url
.
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification, and it looks like your proposed title needs to be adjusted. Details:
|
Closed this draft because we decided to make better solution not in the test framework. #4753 |
Description
Linked issue
Closes #{issue_number}
Benefits
Checklist
CONTRIBUTING.md