-
Notifications
You must be signed in to change notification settings - Fork 44
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
Makes pulp_smash a pytest plugin #1262
Makes pulp_smash a pytest plugin #1262
Conversation
19759b5
to
01c1ff4
Compare
|
||
|
||
@pytest.fixture | ||
def async_monitor_task(tasks_api_client): |
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 still fail to see the value in this being a "fixture".
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 agree. I'm not finished with this one yet. The issue I'm struggling with is that if this becomes a normal function then I can't access the chain of fixtures it depends on tasks_api_client -> pulpcore client fixtures. I'm going to look at this more today.
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.
Oh right. There is a point.
|
||
@pytest.fixture | ||
def tasks_api_client(pulpcore_client): | ||
return TasksApi(pulpcore_client) |
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 pretty much like this. Can we mark it as scope="session"?
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 think this was a very good idea.
d1ea780
to
dee2a3c
Compare
dee2a3c
to
ea9039a
Compare
Required PR: pulp/pulp-smash#1262 closes #9623
Required PR: pulp/pulp-smash#1262 closes #9623
Required PR: pulp/pulp-smash#1262 closes #9623
Required PR: pulp/pulp-smash#1262 closes #9623
008569f
to
6ce1ec5
Compare
Required PR: pulp/pulp-smash#1262 closes #9623
Required PR: pulp/pulp-smash#1262 closes #9623
6ce1ec5
to
b516e90
Compare
Required PR: pulp/pulp-smash#1262 closes #9623
Required PR: pulp/pulp-smash#1262 closes #9623
Required PR: pulp/pulp-smash#1262 closes #9623
Required PR: pulp/pulp-smash#1262 closes #9623
Required PR: pulp/pulp-smash#1262 closes #9623
b516e90
to
cd77852
Compare
Required PR: pulp/pulp-smash#1262 closes #9623
Required PR: pulp/pulp-smash#1262 closes #9623
cd77852
to
a0e2657
Compare
Required PR: pulp/pulp-smash#1262 closes #9623
bc72bbd
to
87d4427
Compare
Required PR: pulp/pulp-smash#1262 closes #9623
Required PR: pulp/pulp-smash#1262 closes #9623
48137c9
to
724376e
Compare
Required PR: pulp/pulp-smash#1262 closes #9623
Required PR: pulp/pulp-smash#1262 closes #9623
724376e
to
8faee76
Compare
Required PR: pulp/pulp-smash#1262 closes #9623
Required PR: pulp/pulp-smash#1262 closes #9623
Required PR: pulp/pulp-smash#1262 closes #9623
@pytest.fixture | ||
def delete_orphans_post(): | ||
yield | ||
delete_orphans() | ||
|
||
|
||
@pytest.fixture | ||
def delete_orphans_pre_post(delete_orphans_pre, delete_orphans_post): | ||
yield |
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 kind of think we should delete these two, and have everyone use delete_orphans_pre
. Doing it both before and after for each test is a lot and will slow us down. Additionally, I'm not even sure the post will free up actual orphans due to the run order. After the test finishes and it's tearing down all of the fixtures, if the delete_orphans_post
runs before the deletion of the repository itself, it won't even free the orphans. Rather than worry about fixing that, let's just use delete_orphans_pre
. @mdellweg wdyt?
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.
sounds good. We could depend other fixtures on this one to get the order right.
(cleanup is in reverse dependency order.)
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.
Agreed. I made this change and the runtime dropped by a few more seconds for the 7 tests I've been running. It also removed one unecessary fixture in the other PR too.
8faee76
to
2234680
Compare
Required PR: pulp/pulp-smash#1262 closes #9623
Required PR: pulp/pulp-smash#1262 closes #9623
Required PR: pulp/pulp-smash#1262 closes #9623
Required PR: pulp/pulp-smash#1262 closes #9623
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, we need the conftest.py in this repo. But probably does not hurt also.
|
||
|
||
@pytest.fixture | ||
def gen_object_with_cleanup(): |
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.
Maybe gen_objects_with_cleanup_async_delete
?
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 would do autodetection whether there is waiting on tasks needed.
2234680
to
2a157d2
Compare
This provides a common place for any pytest fixtures to be added.
2a157d2
to
c797c74
Compare
Required PR: pulp/pulp-smash#1262 closes #9623
import socket | ||
import ssl | ||
|
||
from dataclasses import dataclass |
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 breaks compatibility with Python 3.6
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.
Hmmm, maybe I need to rework this so it doesn't break old release branches....
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 got addressed a04c4ee
This provides a common place for any pytest fixtures to be added.