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

GOTH test for renegotiating proposals #1498

Merged
merged 4 commits into from Aug 2, 2021
Merged

GOTH test for renegotiating proposals #1498

merged 4 commits into from Aug 2, 2021

Conversation

jiivan
Copy link
Contributor

@jiivan jiivan commented Jul 9, 2021

fixes: #1423

# Version after fix applied (should pass)
GITHUB_API_TOKEN=<XXX>  poetry run pytest -svx domain/ya-provider/test_provider_multi_activity.py::test_provider_renegotiate_proposal --config-override docker-compose.build-environment.commit-hash=54917c0bc383613afaf5cfc34279035a142fee58

# Version before fix applied (should fail)
GITHUB_API_TOKEN=<XXX>  poetry run pytest -svx domain/ya-provider/test_provider_multi_activity.py::test_provider_renegotiate_proposal --config-override docker-compose.build-environment.commit-hash=224532666626988377305665574fc6ed45f81f85

@jiivan jiivan requested a review from kmazurek July 9, 2021 10:56
@jiivan jiivan linked an issue Jul 9, 2021 that may be closed by this pull request
GOTH test for renegotiating proposals #1423
Closed
2 tasks
kmazurek
kmazurek reviewed Jul 9, 2021
View reviewed changes
goth_tests/domain/ya-provider/test_provider_multi_activity.py
.build()
)

async def negotiate_begin(requestor, demand, providers):
Copy link
Contributor

@kmazurek kmazurek Jul 9, 2021 •
edited

Choose a reason for hiding this comment

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

My initial thought here: at least some of these internal helper functions can be moved to goth_tests/helpers/negotiation.py (once the PR is considered ready).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about it, but I think that this helpers are specific to renegotiation test. The commonly used helper is goth_tests.helpers.negotiation.negotiate_agreements(). I can't think of a common use for those custom helpers.

tworec
tworec suggested changes Jul 23, 2021
View reviewed changes
goth_tests/domain/ya-provider/test_provider_multi_activity.py Outdated Show resolved Hide resolved
goth_tests/domain/ya-provider/test_provider_multi_activity.py Outdated Show resolved Hide resolved
goth_tests/domain/ya-provider/test_provider_multi_activity.py Outdated Show resolved Hide resolved
goth_tests/domain/ya-provider/test_provider_multi_activity.py Outdated Show resolved Hide resolved
goth_tests/domain/ya-provider/test_provider_multi_activity.py Outdated
subscription_id
)
logger.info("collected offers: %s", collected_offers)
assert len(collected_offers) == 2
Copy link
Contributor

Choose a reason for hiding this comment

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

This binds invocation of this fn to the very specific place within the scenario. If invoked before requestor-1 is finished we will get only 1 proposal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the reneegotiate() should be called after first requestor finished.

goth_tests/domain/ya-provider/test_provider_multi_activity.py Show resolved Hide resolved
goth_tests/domain/ya-provider/test_provider_multi_activity.py
async with runner(config.containers):
requestor1, requestor2 = runner.get_probes(probe_type=RequestorProbe)
providers = runner.get_probes(probe_type=ProviderProbe)
assert providers
Copy link
Contributor

Choose a reason for hiding this comment

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

I've added this line as @zakaprov did in above tests, and indeed providers was empty list

goth_tests/domain/ya-provider/test_provider_multi_activity.py Show resolved Hide resolved
goth_tests/domain/ya-provider/test_provider_multi_activity.py Show resolved Hide resolved
@jiivan jiivan requested a review from tworec July 29, 2021 17:44
@tworec tworec merged commit d4295be into release/v0.7 Aug 2, 2021
@tworec tworec deleted the market/0.7-renego branch August 2, 2021 15:40
@kmazurek
Copy link
Contributor

kmazurek commented Aug 2, 2021

@tworec I didn't get a chance to review these changes before merging. @jiivan How does this relate to golemfactory/yapapi#551 ?

@jiivan
Copy link
Contributor Author

jiivan commented Aug 3, 2021

@tworec I didn't get a chance to review these changes before merging. @jiivan How does this relate to golemfactory/yapapi#551 ?

:( This part tests a scenario where provider rejects proposal and then renegotiates. In yapapi, requestor rejects proposal and then renegotiates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kmazurek kmazurek kmazurek left review comments

@tworec tworec tworec approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

GOTH test for renegotiating proposals
3 participants
@jiivan @kmazurek @tworec