Rewrite `test_filter.py` using pytest #328

git-hyagi · 2024-01-15T15:55:19Z

fixes: #324

mdellweg · 2024-01-15T16:07:37Z

pulp_ostree/tests/functional/conftest.py

+@pytest.fixture()
+def setup_filter(
+    repo,
+    remote,
+    ostree_repository_version,
+    ostree_repositories_api,
+    ostree_remotes_api,
+):
+    """Setup for ostree filter tests"""
+    delete_orphans()
+
+    yield ostree_repository_version
+    ostree_repositories_api.delete(repo.pulp_href)
+    ostree_remotes_api.delete(remote.pulp_href)
+    delete_orphans()


If this is a special setup code for a few tests in one module, the fixture should live there too.
Instead of calling delete_orphans, use the delete_orphans_pre fixture (I always suppose to check if it's needed at all...).

mdellweg · 2024-01-15T16:09:52Z

pulp_ostree/tests/functional/conftest.py

+from pulp_smash.pulp3.bindings import delete_orphans, monitor_task
+from pulp_smash.pulp3.utils import gen_repo


One of the rewrite goals is to stop using pulp-smash...
Both delete_orphans (as delete_orphans_pre) and monitor_task are provided by fixtures.
gen_repo is not doing much more than creating a dict with a "name" key.

lubosmj

Thanks for opening this PR! I have added a couple of suggestions.

lubosmj · 2024-01-16T22:52:20Z

pulp_ostree/tests/functional/conftest.py

+def ostree_client(_api_client_set, bindings_cfg):
+    """Fixture to provide a client to Pulp API"""
+    api_client = ApiClient(bindings_cfg)
+    _api_client_set.add(api_client)
+    yield api_client
+    _api_client_set.remove(api_client)


Is it possible to keep this simple?

from pulpcore.client.pulp_ostree import ApiClient as OstreeApiClient @pytest.fixture(scope="session") def ostree_client(bindings_cfg): """Fixture for an OSTree client.""" return OstreeApiClient(bindings_cfg)

Note that in this case I am using the session scope which means that the fixture is destroyed at the end of the test session. This is what we usually want when dealing with generic fixtures that are used from multiple test modules. In case we want to execute the fixture each time being requested, we leave the scope unfilled. Would you mind using the session scope across all the relevant fixtures, please?

I agree with the session scope. However registering this client with the _api_client_set what provides our tests with a unique correlation id per test. It proved to be helpful in debugging.

Sounds good. We do not use such a thing in pulp-rpm.

lubosmj · 2024-01-16T22:58:16Z

pulp_ostree/tests/functional/conftest.py

+@pytest.fixture()
+def ostree_repository_sync_url(ostree_client):
+    """Fixture that returns an instance of RepositorySyncURL"""
+    return RepositorySyncURL(ostree_client)


Creating a fixture for RepositorySyncURL is not necessary because this class is a wrapper for the body of a request sent to sync remote content. Usually, we use it in the following way: https://github.com/pulp/pulp_rpm/blob/6d331892b3688b5eea574d7b594c92a5227b8674/pulp_rpm/tests/functional/api/test_publish.py#L435-L438.

lubosmj · 2024-01-16T23:00:55Z

pulp_ostree/tests/functional/conftest.py

+def remote(ostree_remotes_api):
+    """Fixture that creates an ostree Remote and returns the object created"""
+    body = gen_ostree_remote(depth=0, policy="immediate")
+    return ostree_remotes_api.create(body)


Instead of writing this, you may want to implement a factory that will handle the setup and cleanup of resources by itself by leveraging gen_object_with_cleanup, like so: https://github.com/pulp/pulp_gem/blob/4de8c2adf1be973958d95b16f0c4137dab7ba066/pulp_gem/tests/functional/conftest.py#L105-L116. At the moment, we do not support domains, so you can skip that part.

Do you think it makes sense to follow the same concept for the corresponding repo and distribution fixture?

lubosmj · 2024-01-16T23:02:43Z

pulp_ostree/tests/functional/conftest.py

+@pytest.fixture()
+def random_name():
+    """Fixture that returns a dictionary with a random uuid for the `name` key"""
+    return {"name": str(uuid.uuid4())}


I am in favour of removing the fixture. From time to time, we want to find a balance between the usability of the fixture and self-explanatory code used directly in a test case.

lubosmj · 2024-01-16T23:14:15Z

pulp_ostree/tests/functional/conftest.py

+
+
+@pytest.fixture()
+def ostree_repository_version(


Let's clearly describe what the fixture returns.

Suggested change

def ostree_repository_version(

def synced_repo_version(

Furthermore, you may find it useful to call ostree_repo_factory here.

lubosmj · 2024-01-16T23:16:58Z

pulp_ostree/tests/functional/conftest.py

+@pytest.fixture()
+def ostree_repositories_delete(ostree_repositories_api, repo):
+    """Fixture that deletes an ostree Repository"""
+    return ostree_repositories_api.delete(repo.pulp_href)
+
+
+@pytest.fixture()
+def ostree_remotes_delete(ostree_remotes_api, remote):
+    """Fixture that deletes an ostree Remote"""
+    return ostree_remotes_api.delete(remote.pulp_href)


With pytest, you do not need to care about cleanups in the old-fashioned way. The procedure is as follows:

def repo_fixture(): repo = setup_repo() yield repo delete_repo(repo)

Thus, by following the logic implemented here: https://github.com/pulp/pulp_gem/blob/4de8c2adf1be973958d95b16f0c4137dab7ba066/pulp_gem/tests/functional/conftest.py#L105-L116, we can clearly get rid of these two cleanup fixtures.

lubosmj

I left two more comments. Please, review them.

In addition to that, have you tried dropping delete_orphans_pre? In my opinion, it is not required.

lubosmj · 2024-01-17T16:30:32Z

pulp_ostree/tests/functional/api/test_filter.py

+    delete_orphans_pre,
+):
+    """Check if refs can be filtered by their names and related commits' checksums."""
+    repo = synced_repo()


Will this fixture be ever called with other than non-default parameters? How about making it a regular fixture that returns an object instead of a function?

Oh! I see!
Good point!

lubosmj · 2024-01-17T16:34:07Z

pulp_ostree/tests/functional/api/test_filter.py

+):
+    """Check if commits can be filtered by their checksums."""
+    repo = synced_repo()
+    assert repo.latest_version_href == f"{repo.pulp_href}versions/1/"


We know that the repository is synced. There is no need to assert on the version number.

git-hyagi · 2024-01-17T17:25:43Z

In addition to that, have you tried dropping delete_orphans_pre?

Hum… I didn't. I'll do some tests without it before updating the PR.

lubosmj

Do you believe it makes sense to run these tests in parallel?

pytest -v -r sx --color=yes --suppress-no-test-exit-code --pyargs pulp_ostree.tests.functional -m parallel -n 8

https://github.com/pulp/pulp_gem/blob/4de8c2adf1be973958d95b16f0c4137dab7ba066/pulp_gem/tests/functional/api/test_sync.py#L36

lubosmj · 2024-01-17T17:38:10Z

pulp_ostree/tests/functional/api/test_filter.py

+    def _synced_repo():
+        repo = ostree_repository_factory()
+        remote = ostree_remote_factory(url=OSTREE_FIXTURE_URL, policy="immediate")
+        result = ostree_repositories_api_client.sync(repo.pulp_href, {"remote": remote.pulp_href})
+        monitor_task(result.task)
+        return ostree_repositories_api_client.read(repo.pulp_href)
+


There is no need for having an inner function anymore. We can simply return ostree_repositories_api_client.read(repo.pulp_href) from synced_repo.

git-hyagi · 2024-01-17T17:58:11Z

Do you believe it makes sense to run these tests in parallel?

Hum... I guess it does.
From what I could understand, the filter_refs and filter_commits work in different repositories (each one creates its own repository), therefore it should be fine to run them in parallel.

fixes: pulp#324

mdellweg reviewed Jan 15, 2024

View reviewed changes

git-hyagi force-pushed the pytest-refactor-filter branch from c267c5e to cd02c5a Compare January 15, 2024 18:17

lubosmj requested changes Jan 16, 2024

View reviewed changes

git-hyagi force-pushed the pytest-refactor-filter branch from cd02c5a to 2ca657b Compare January 17, 2024 14:04

git-hyagi marked this pull request as draft January 17, 2024 14:17

git-hyagi force-pushed the pytest-refactor-filter branch from 2ca657b to eba23fd Compare January 17, 2024 15:43

git-hyagi marked this pull request as ready for review January 17, 2024 15:53

lubosmj reviewed Jan 17, 2024

View reviewed changes

git-hyagi force-pushed the pytest-refactor-filter branch from eba23fd to 0ac19cd Compare January 17, 2024 17:35

lubosmj reviewed Jan 17, 2024

View reviewed changes

Rewrite test_filter.py using pytest

afd04ee

fixes: pulp#324

git-hyagi force-pushed the pytest-refactor-filter branch from 0ac19cd to afd04ee Compare January 17, 2024 18:00

lubosmj approved these changes Jan 17, 2024

View reviewed changes

lubosmj merged commit 4273c0f into pulp:main Jan 17, 2024
16 checks passed

git-hyagi deleted the pytest-refactor-filter branch January 17, 2024 21:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite `test_filter.py` using pytest #328

Rewrite `test_filter.py` using pytest #328

git-hyagi commented Jan 15, 2024

mdellweg Jan 15, 2024

mdellweg Jan 15, 2024

lubosmj left a comment

lubosmj Jan 16, 2024

mdellweg Jan 17, 2024

lubosmj Jan 17, 2024

lubosmj Jan 16, 2024

lubosmj Jan 16, 2024

lubosmj Jan 16, 2024

lubosmj Jan 16, 2024

lubosmj Jan 16, 2024

lubosmj left a comment

lubosmj Jan 17, 2024

git-hyagi Jan 17, 2024

lubosmj Jan 17, 2024

git-hyagi commented Jan 17, 2024

lubosmj left a comment

lubosmj Jan 17, 2024

git-hyagi commented Jan 17, 2024

		from pulp_smash.pulp3.bindings import delete_orphans, monitor_task
		from pulp_smash.pulp3.utils import gen_repo

Rewrite test_filter.py using pytest #328

Rewrite test_filter.py using pytest #328

Conversation

git-hyagi commented Jan 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lubosmj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lubosmj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

git-hyagi commented Jan 17, 2024

lubosmj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

git-hyagi commented Jan 17, 2024

Rewrite `test_filter.py` using pytest #328

Rewrite `test_filter.py` using pytest #328