Skip to content
This repository has been archived by the owner on Dec 7, 2022. It is now read-only.

Adjust CopyRecursiveUnitsTestCase to use recursive_conservative #159

Merged
merged 1 commit into from Feb 4, 2019

Conversation

nixocio
Copy link
Contributor

@nixocio nixocio commented Jan 31, 2019

Pulp 2.18.1 introduced a new flag recursive_conservative. Adjust the
test case CopyRecursiveUnitsTestCase to use this new flag.

recursive_conservative is set to False by default.

See: https://pulp.plan.io/issues/4152
See: https://pulp.plan.io/issues/4269
See: https://pulp.plan.io/issues/4375

@nixocio
Copy link
Contributor Author

nixocio commented Jan 31, 2019

Tested against 2.18.1b1 installation RHEL7.6.

@dralley, and @goosemania, please confirm this is the expected behaviour.

@nixocio nixocio requested a review from bherrin3 January 31, 2019 15:57
Copy link
Contributor

@dralley dralley left a comment

Choose a reason for hiding this comment

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

Looks good I think

@nixocio nixocio changed the title Adjust CopyRecursiveUnitsTestCase to use recursive_conservative [DONOTMERGE] Adjust CopyRecursiveUnitsTestCase to use recursive_conservative Jan 31, 2019
@@ -199,9 +201,11 @@ def setUpClass(cls):
def test_recursive(self):
"""Test recursive copy for a repository with rich/weak dependencies.

In order to copy all dependencies rich and weak ones, the flag
``recurisve_conservative`` has to be set as True.
Copy link
Member

Choose a reason for hiding this comment

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

@dralley , @kersommoura , I don't think that's right.
If rich dependencies are resolved only for conservative case, I think it's a bug in solver

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@goosemania, thanks.

Copy link
Contributor

@dralley dralley Feb 1, 2019

Choose a reason for hiding this comment

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

@kersommoura Could you add the "True, False" test case here? I plan to look into this bug this afternoon hopefully.

edit: ah, didn't notice you're on PTO

Copy link
Contributor

Choose a reason for hiding this comment

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

You can attach this issue in the description https://pulp.plan.io/issues/4375

Copy link
Contributor

@dralley dralley Feb 2, 2019

Choose a reason for hiding this comment

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

I fixed the issue here: pulp/pulp_rpm#1261

These tests pass with my new PR. Here's a diff which tests the recursive flag, alone.

--- a/pulp_2_tests/tests/rpm/api_v2/test_rich_weak_dependencies.py
+++ b/pulp_2_tests/tests/rpm/api_v2/test_rich_weak_dependencies.py
@@ -201,8 +201,22 @@ class CopyRecursiveUnitsTestCase(unittest.TestCase):
     def test_recursive(self):
         """Test recursive copy for a repository with rich/weak dependencies.
 
-        In order to copy all dependencies rich and weak ones, the flag
-        ``recurisve_conservative`` has to be set as True.
+        See :meth:`do_test`."
+        """
+        repo = self.do_test(True, False)
+        dst_unit_ids = [
+            unit['metadata']['name'] for unit in
+            search_units(self.cfg, repo, {'type_ids': ['rpm']})
+        ]
+        self.assertEqual(
+            len(dst_unit_ids),
+            RPM2_RICH_WEAK_DATA['total_installed_packages'],
+            dst_unit_ids
+        )
+
+    def test_recursive_conservative(self):
+        """Test recursive copy for a repository with rich/weak dependencies.
+
         See :meth:`do_test`."
         """
         repo = self.do_test(True, True)

@nixocio nixocio changed the title [DONOTMERGE] Adjust CopyRecursiveUnitsTestCase to use recursive_conservative Adjust CopyRecursiveUnitsTestCase to use recursive_conservative Feb 4, 2019
@nixocio
Copy link
Contributor Author

nixocio commented Feb 4, 2019

@dralley, thanks. I applied the patch.

@nixocio nixocio force-pushed the fix_rich_copy branch 2 times, most recently from cfcffee to 58b1a0b Compare February 4, 2019 19:01
Pulp 2.18.1 introduced a new flag `recursive_conservative`. Adjust the
test case CopyRecursiveUnitsTestCase to use this new flag.

`recursive_conservative` is set to False by default.

See: https://pulp.plan.io/issues/4152
See: https://pulp.plan.io/issues/4269
See: https://pulp.plan.io/issues/4375
@nixocio
Copy link
Contributor Author

nixocio commented Feb 4, 2019

Tested against Platform Version: 2.18.1b1 RHEL7.6

@nixocio
Copy link
Contributor Author

nixocio commented Feb 4, 2019

Tested against Platform Version: 2.17.1 RHEL7.6.

Copy link
Contributor

@bherrin3 bherrin3 left a comment

Choose a reason for hiding this comment

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

Reviewed against RHEL7.6 FIPS and non-FIPS configured Pulp 2.18.1b1 with 4375 merged and 2.19master2

All linting and testing passed, with the expected failure outside the scope of this PR.

@bherrin3 bherrin3 merged commit 5da698e into pulp:master Feb 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants