Rewrite `test_modify.py` using pytest #333

git-hyagi · 2024-01-23T19:39:16Z

fixes: #326

lubosmj

Keep on! Do you mind addressing the comments?

lubosmj · 2024-01-24T14:45:29Z

pulp_ostree/tests/functional/api/test_modify.py

+@pytest.fixture
+def synced_repo_version(
+    ostree_repositories_api_client,
+    ostree_repositories_versions_api_client,
+    ostree_repository_factory,
+    ostree_remote_factory,
+    monitor_task,
+):
+    """
+    Fixture that syncs a Remote ostree Repository,
+    waits until the sync task completes and returns the
+    created repository version object.
+    """
+    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})
+    repo_version_href = monitor_task(result.task).created_resources[0]
+    return ostree_repositories_versions_api_client.read(repo_version_href), remote, repo


Is this a duplicate of

pulp_ostree/pulp_ostree/tests/functional/api/test_filter.py

Line 7 in b148ec4

def synced_repo(

? If so, we can re-use that, right?

Yes, kind of the same but instead of returning the repo we are now returning also the repo_version and remote.
At first, I thought that reusing it (by adding some conditionals or modifying the return) would compromise the readability/maintainability, but thinking again maybe not ... I'll check it!

lubosmj · 2024-01-24T15:02:22Z

pulp_ostree/tests/functional/api/test_modify.py

+
+
+@pytest.fixture
+def modify_repo(bindings_cfg, monitor_task):


I would remove this fixture. Try using repository_api.modify() instead. It looks much more appealing!

https://github.com/pulp/pulpcore/blob/e2119307497f401de95e463e370787143c524016/pulpcore/tests/functional/api/test_repos.py#L39

lubosmj · 2024-01-24T15:31:18Z

pulp_ostree/tests/functional/api/test_modify.py

+
+
+@pytest.fixture
+def get_content(bindings_cfg):


I believe you can delete this.

In the past, we replaced get_content from pulp_smash by a plain call to content_api.list(repo.latest_version), see pulp/pulp_file@5c65d92#diff-082889e4107e892cad9e326317979e0a6e6b82a021fb85db8228af551797d6a0R99. Considering this, I propose adopting a similar strategy in the current context.

Furthermore, we will need to modify or remove the normalize_content function to parse results separately for each content type. Alternatively, we can make the function a fixture and fuse the content_api.list(repo.latest_version) logic into it, so we will have the downloading and normalizing at one place. What do you think?

hum... not sure if I understood it, but both options seem complicated. I'll do some tests and revisit/re-read the functions to have a better understanding of the proposals.

The basic idea is to use bindings to get things done. We should stray away from using requests to access API endpoints that are suited for administering. In this case, we want to get a list of content units from an API endpoint which is properly mapped by the bindings.

lubosmj

Lint errors should be fixed now. Please, rebase your commit on top of the main branch.

lubosmj · 2024-01-26T21:50:19Z

pulp_ostree/tests/functional/api/test_modify.py

+        commits = ostree_content_commits_api_client.list(repository_version=repo).results
+        configs = ostree_content_configs_api_client.list(repository_version=repo).results
+        objects = ostree_content_objects_api_client.list(repository_version=repo).results
+        refs = ostree_content_refs_api_client.list(repository_version=repo).results
+        summaries = ostree_content_summaries_api_client.list(repository_version=repo).results


Is there a guarantee that the results are always returned in the same order?

We used sorted in the past to enforce the order:

sorted( [remove_created_key(item) for item in repo_content[OSTREE_OBJECTS_NAME]], key=lambda item: item["pulp_href"], )

hum... the only test that uses the normalize_content is the test_copy_whole_repository and, if I understood it right, the test will not create multiple contents (commits/configs/objects/refs/summaries), therefore the results will always be ordered (a list with 0 or one element is an ordered list), right?

Anyway, I'll order it so that we can use it in the future with other tests if necessary.

The synced repository should contain 3 objects as far as I know.

ohhh ...my bad! (I still need to study and learn ostree to understand what is going on)

lubosmj · 2024-01-26T21:54:44Z

pulp_ostree/tests/functional/api/test_modify.py

+    response = ostree_repositories_api_client.modify(
+        ostree_ostree_repository_href=repo1.pulp_href,
+        repository_add_remove_content={"add_content_units": [obj["pulp_href"]]},
+    )
+    monitor_task(response.task)
+    version2 = repo1.latest_version_href


It is necessary to re-load the version object by doing ostree_repositories_api_client.read(repo1.pulp_href).latest_version_href. We aim to verify if the repository version was really not changed.

lubosmj · 2024-01-26T21:55:45Z

pulp_ostree/tests/functional/api/test_modify.py

+        repository_add_remove_content={"remove_content_units": [obj["pulp_href"]]},
+    )
+    monitor_task(response.task)
+    version2 = repo1.latest_version_href


Same here. The latest version is not automatically updated for repo1 since it represent an older state of the repository, before running the modify task.

lubosmj · 2024-01-26T21:57:23Z

pulp_ostree/tests/functional/conftest.py

+    result = ostree_repositories_api_client.sync(repo.pulp_href, {"remote": remote.pulp_href})
+    repo_version_href = monitor_task(result.task).created_resources[0]
+    repo = ostree_repositories_api_client.read(repo.pulp_href)
+    return ostree_repositories_versions_api_client.read(repo_version_href), remote, repo


It might be worth returning the updated repository, containing a new latest repository version after syncing.

repo = ostree_repositories_api_client.read(repo.pulp_href)

hum... I didn't get it. I thought that lines 143
https://github.com/pulp/pulp_ostree/pull/333/files/f027c34e99b30bb933276d2c24f29ad50ff97b8c#diff-fb6fcf86eeb05753e592d20f35257cdb41de6529cd8deb07781fb7707b8e378fR143

and return ... , ..., repo (line144) would do that
https://github.com/pulp/pulp_ostree/pull/333/files/f027c34e99b30bb933276d2c24f29ad50ff97b8c#diff-fb6fcf86eeb05753e592d20f35257cdb41de6529cd8deb07781fb7707b8e378fR144

Oh, my bad. This one is correct. It went unnoticed.

fixes: pulp#326

git-hyagi force-pushed the pytest-refactor-modify branch from f08d3f7 to b66dde9 Compare January 24, 2024 10:49

lubosmj requested changes Jan 24, 2024

View reviewed changes

git-hyagi force-pushed the pytest-refactor-modify branch 2 times, most recently from 86f50d6 to f027c34 Compare January 26, 2024 13:40

lubosmj requested changes Jan 26, 2024

View reviewed changes

git-hyagi force-pushed the pytest-refactor-modify branch from f027c34 to e09f13c Compare January 29, 2024 12:28

Rewrite test_modify.py using pytest

068d70d

fixes: pulp#326

git-hyagi force-pushed the pytest-refactor-modify branch from e09f13c to 068d70d Compare January 30, 2024 10:25

lubosmj approved these changes Jan 30, 2024

View reviewed changes

lubosmj merged commit 978fac4 into pulp:main Jan 30, 2024
16 checks passed

git-hyagi deleted the pytest-refactor-modify branch January 30, 2024 13:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite `test_modify.py` using pytest #333

Rewrite `test_modify.py` using pytest #333

git-hyagi commented Jan 23, 2024

lubosmj left a comment

lubosmj Jan 24, 2024

git-hyagi Jan 24, 2024

lubosmj Jan 24, 2024

lubosmj Jan 24, 2024

git-hyagi Jan 24, 2024

lubosmj Jan 25, 2024

lubosmj left a comment

lubosmj Jan 26, 2024

git-hyagi Jan 29, 2024

lubosmj Jan 29, 2024

git-hyagi Jan 29, 2024

lubosmj Jan 26, 2024

lubosmj Jan 26, 2024

lubosmj Jan 26, 2024

git-hyagi Jan 29, 2024

lubosmj Jan 29, 2024

Rewrite test_modify.py using pytest #333

Rewrite test_modify.py using pytest #333

Conversation

git-hyagi commented Jan 23, 2024

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

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

Choose a reason for hiding this comment

Rewrite `test_modify.py` using pytest #333

Rewrite `test_modify.py` using pytest #333