-
Notifications
You must be signed in to change notification settings - Fork 4
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
Impl for release allocations feature tests #580
Conversation
goth/runner/probe/mixin.py
Outdated
return allocation_result | ||
|
||
@step() | ||
async def get_allocation(self: ProbeProtocol, allocation: Allocation) -> Allocation: |
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.
Since we're not using the full Allocation
object here for anything else other than extracting its ID - let's just use the ID as the function's parameter directly:
async def get_allocation(self: ProbeProtocol, allocation_id: str) -> Allocation:
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.
Thank you for your review. Impl done
goth/runner/probe/mixin.py
Outdated
) | ||
|
||
allocation_result = await self.api.payment.create_allocation(allocation) | ||
logger.debug("Created allocation. id=%s", allocation_result.allocation_id) |
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 noticed the allocation creation logic here is analogous to the method pay_invoices
from this mixin. Perhaps we could move that logic to a separate function (something like _create_allocation
) and just parametrize what's needed?
That would also resolve the inconsistency in logging between the two step methods - pay_invoices
is logging the entire Allocation
object (line 263), while create_allocation
extracts just the ID (line 290).
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.
Thank you for your review. Impl done
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.
Nice! Thanks for applying the suggested changes. :)
This PR is related to new test implementation in goth-tests regarding release allocation feature. Please check this PR for more datails.
It fixes golemfactory/yagna#1246 due to new test implementation.