Skip to content

Conversation

@evgeni
Copy link
Member

@evgeni evgeni commented Oct 25, 2024

No description provided.

@evgeni evgeni marked this pull request as draft October 25, 2024 12:06
Comment on lines +36 to +48
def yum_repository(product, organization, foremanapi):
repo = foremanapi.create('repositories', {'name': str(uuid.uuid4()), 'product_id': product['id'], 'content_type': 'yum', 'url': 'https://fixtures.pulpproject.org/rpm-no-comps/'})
Copy link
Member

Choose a reason for hiding this comment

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

I don't know how far you want to take this, but I think you can even make the URL a parameter so it can be overridden

Suggested change
def yum_repository(product, organization, foremanapi):
repo = foremanapi.create('repositories', {'name': str(uuid.uuid4()), 'product_id': product['id'], 'content_type': 'yum', 'url': 'https://fixtures.pulpproject.org/rpm-no-comps/'})
def yum_repository(product, organization, foremanapi, url='https://fixtures.pulpproject.org/rpm-no-comps/'):
repo = foremanapi.create('repositories', {'name': str(uuid.uuid4()), 'product_id': product['id'], 'content_type': 'yum', 'url': url})

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think fixtures take regular parameters like this (there are fixture factories, but those are more complicated)

Copy link
Member

Choose a reason for hiding this comment

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

https://docs.pytest.org/en/7.1.x/example/parametrize.html#indirect-parametrization says you can do it, but you need to use request.param. Feel free to leave it out for now, but let's keep it in mind for the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh via indirect… Mhh, interesting. Maybe. I'll think about it.

Copy link
Member Author

Choose a reason for hiding this comment

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

#60 is to track this, merging as is for now

Comment on lines +54 to +66
def lifecycle_environment(organization, foremanapi):
library = foremanapi.list('lifecycle_environments', 'name=Library', {'organization_id': organization['id']})[0]
Copy link
Member

Choose a reason for hiding this comment

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

Same here perhaps?

Suggested change
def lifecycle_environment(organization, foremanapi):
library = foremanapi.list('lifecycle_environments', 'name=Library', {'organization_id': organization['id']})[0]
def lifecycle_environment(organization, foremanapi, prior='Library'):
library = foremanapi.list('lifecycle_environments', f'name={prior}', {'organization_id': organization['id']})[0]

Though perhaps it should just accept an instance where None is automatically resolve to the library:

Suggested change
def lifecycle_environment(organization, foremanapi):
library = foremanapi.list('lifecycle_environments', 'name=Library', {'organization_id': organization['id']})[0]
def lifecycle_environment(organization, foremanapi, prior=None):
if not prior:
prior = foremanapi.list('lifecycle_environments', 'name=Library', {'organization_id': organization['id']})[0]

Which also makes me question what the Katello API does if no prior ID is given. It's not listed as required and mandating this, but would it make sense to find the library server side and make it an optional parameter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I meant that last part as an RFE.

)

@pytest.fixture
def organization(foremanapi):
Copy link
Member Author

Choose a reason for hiding this comment

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

Right now all these tests have an (implicit) scope=function (https://docs.pytest.org/en/stable/how-to/fixtures.html#fixture-scopes) which means that they are destroyed at the end of every test.

Conceptually, this is good, of course. But on the flip side this means that every repository test also first creates a new org (also in Candlepin), a new product, etc. Those times quickly add up, and make the test tiresome to execute.

We can speed this up with scope=module, but this means that any failure in a test might have cascading effects on later tests (which is the same behaviour as we have today with bats, FWIW).

WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

For now I'd lean to the isolation given we still have very few tests, but I agree in the future it will add up. Later we can add a "persistent" fixture that can be reused.

Given we have isolation, it's probably also easy to run things in parallel. Those failures may be harder to diagnose, but it can also uncover real work concurrency issues which large deployments will see at some point.

Comment on lines +54 to +55
vagrant up quadlet
vagrant up client
Copy link
Member

Choose a reason for hiding this comment

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

Do you intentionally start them up sequentially instead of parallel? Is that the issue with fetching the same box concurrently giving issues?

Copy link
Member Author

Choose a reason for hiding this comment

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

I honestly haven't tried 😅

@evgeni evgeni force-pushed the apifixtures branch 4 times, most recently from 31c26f3 to d926760 Compare November 18, 2024 14:04
@ehelms
Copy link
Member

ehelms commented Nov 18, 2024

Generally looks good.. what's needed to get it out of draft?

@evgeni
Copy link
Member Author

evgeni commented Nov 19, 2024

Generally looks good.. what's needed to get it out of draft?

@evgeni evgeni marked this pull request as ready for review November 19, 2024 11:52
hosts: all
become: true
roles:
- theforeman.forklift.etc_hosts
Copy link
Member

Choose a reason for hiding this comment

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

🎆

@evgeni evgeni merged commit 177348f into theforeman:master Nov 20, 2024
3 checks passed
@evgeni evgeni deleted the apifixtures branch November 20, 2024 14:52
@evgeni evgeni mentioned this pull request Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants