installer: remove unrequested extras if requires_synchronization is set, increase test coverage of installer regarding extras

Author: radoering

src/poetry/installation/installer.py

@@ -267,7 +267,7 @@ def _do_install(self) -> int:
             ops = self._get_operations_from_lock(locked_repository)
 
         lockfile_repo = LockfileRepository()
-        self._populate_lockfile_repo(lockfile_repo, ops)
+        uninstalls = self._populate_lockfile_repo(lockfile_repo, ops)
 
         if not self.executor.enabled:
             # If we are only in lock mode, no need to go any further
@@ -324,6 +324,8 @@ def _do_install(self) -> int:
                 for op in transaction.calculate_operations(with_uninstalls=True)
                 if op.job_type == "uninstall"
             ] + ops
+        else:
+            ops = uninstalls + ops
 
         # We need to filter operations so that packages
         # not compatible with the current system,
@@ -359,18 +361,19 @@ def _execute(self, operations: list[Operation]) -> int:
 
     def _populate_lockfile_repo(
         self, repo: LockfileRepository, ops: Iterable[Operation]
-    ) -> None:
+    ) -> list[Uninstall]:
+        uninstalls = []
         for op in ops:
             if isinstance(op, Uninstall):
+                uninstalls.append(op)
                 continue
-            elif isinstance(op, Update):
-                package = op.target_package
-            else:
-                package = op.package
 
+            package = op.target_package if isinstance(op, Update) else op.package
             if not repo.has_package(package):
                 repo.add_package(package)
 
+        return uninstalls
+
     def _get_operations_from_lock(
         self, locked_repository: Repository
     ) -> list[Operation]:

tests/installation/test_installer.py

@@ -1009,73 +1009,20 @@ def test_run_with_dependencies_nested_extras(
     assert locker.written_data == expected
 
 
-def test_run_does_not_install_extras_if_not_requested(
-    installer: Installer, locker: Locker, repo: Repository, package: ProjectPackage
-) -> None:
-    package.extras[canonicalize_name("foo")] = [get_dependency("D")]
-    package_a = get_package("A", "1.0")
-    package_b = get_package("B", "1.0")
-    package_c = get_package("C", "1.0")
-    package_d = get_package("D", "1.1")
-
-    repo.add_package(package_a)
-    repo.add_package(package_b)
-    repo.add_package(package_c)
-    repo.add_package(package_d)
-
-    package.add_dependency(Factory.create_dependency("A", "^1.0"))
-    package.add_dependency(Factory.create_dependency("B", "^1.0"))
-    package.add_dependency(Factory.create_dependency("C", "^1.0"))
-    package.add_dependency(
-        Factory.create_dependency("D", {"version": "^1.0", "optional": True})
-    )
-
-    result = installer.run()
-    assert result == 0
-
-    expected = fixture("extras")
-    # Extras are pinned in lock
-    assert locker.written_data == expected
-
-    # But should not be installed
-    assert installer.executor.installations_count == 3  # A, B, C
-
-
-def test_run_installs_extras_if_requested(
-    installer: Installer, locker: Locker, repo: Repository, package: ProjectPackage
-) -> None:
-    package.extras[canonicalize_name("foo")] = [get_dependency("D")]
-    package_a = get_package("A", "1.0")
-    package_b = get_package("B", "1.0")
-    package_c = get_package("C", "1.0")
-    package_d = get_package("D", "1.1")
-
-    repo.add_package(package_a)
-    repo.add_package(package_b)
-    repo.add_package(package_c)
-    repo.add_package(package_d)
-
-    package.add_dependency(Factory.create_dependency("A", "^1.0"))
-    package.add_dependency(Factory.create_dependency("B", "^1.0"))
-    package.add_dependency(Factory.create_dependency("C", "^1.0"))
-    package.add_dependency(
-        Factory.create_dependency("D", {"version": "^1.0", "optional": True})
-    )
-
-    installer.extras(["foo"])
-    result = installer.run()
-    assert result == 0
-
-    # Extras are pinned in lock
-    expected = fixture("extras")
-    assert locker.written_data == expected
-
-    # But should not be installed
-    assert installer.executor.installations_count == 4  # A, B, C, D
-
-
+@pytest.mark.parametrize("is_locked", [False, True])
+@pytest.mark.parametrize("is_installed", [False, True])
+@pytest.mark.parametrize("with_extras", [False, True])
+@pytest.mark.parametrize("do_sync", [False, True])
 def test_run_installs_extras_with_deps_if_requested(
-    installer: Installer, locker: Locker, repo: Repository, package: ProjectPackage
+    installer: Installer,
+    locker: Locker,
+    repo: Repository,
+    installed: CustomInstalledRepository,
+    package: ProjectPackage,
+    is_locked: bool,
+    is_installed: bool,
+    with_extras: bool,
+    do_sync: bool,
 ) -> None:
     package.extras[canonicalize_name("foo")] = [get_dependency("C")]
     package_a = get_package("A", "1.0")
@@ -1096,48 +1043,38 @@ def test_run_installs_extras_with_deps_if_requested(
 
     package_c.add_dependency(Factory.create_dependency("D", "^1.0"))
 
-    installer.extras(["foo"])
-    result = installer.run()
-    assert result == 0
-
-    expected = fixture("extras-with-dependencies")
-
-    # Extras are pinned in lock
-    assert locker.written_data == expected
-
-    # But should not be installed
-    assert installer.executor.installations_count == 4  # A, B, C, D
-
-
-def test_run_installs_extras_with_deps_if_requested_locked(
-    installer: Installer, locker: Locker, repo: Repository, package: ProjectPackage
-) -> None:
-    locker.locked(True)
-    locker.mock_lock_data(fixture("extras-with-dependencies"))
-    package.extras[canonicalize_name("foo")] = [get_dependency("C")]
-    package_a = get_package("A", "1.0")
-    package_b = get_package("B", "1.0")
-    package_c = get_package("C", "1.0")
-    package_d = get_package("D", "1.1")
-
-    repo.add_package(package_a)
-    repo.add_package(package_b)
-    repo.add_package(package_c)
-    repo.add_package(package_d)
-
-    package.add_dependency(Factory.create_dependency("A", "^1.0"))
-    package.add_dependency(Factory.create_dependency("B", "^1.0"))
-    package.add_dependency(
-        Factory.create_dependency("C", {"version": "^1.0", "optional": True})
-    )
+    if is_locked:
+        locker.locked(True)
+        locker.mock_lock_data(fixture("extras-with-dependencies"))
 
-    package_c.add_dependency(Factory.create_dependency("D", "^1.0"))
+    if is_installed:
+        installed.add_package(package_a)
+        installed.add_package(package_b)
+        installed.add_package(package_c)
+        installed.add_package(package_d)
 
-    installer.extras(["foo"])
+    if with_extras:
+        installer.extras(["foo"])
+    installer.requires_synchronization(do_sync)
     result = installer.run()
     assert result == 0
 
-    assert installer.executor.installations_count == 4  # A, B, C, D
+    if not is_locked:
+        assert locker.written_data == fixture("extras-with-dependencies")
+
+    if with_extras:
+        # A, B, C, D
+        expected_installations_count = 0 if is_installed else 4
+        expected_removals_count = 0
+    else:
+        # A, B
+        expected_installations_count = 0 if is_installed else 2
+        # We only want to uninstall extras if we do a "poetry install" without extras,
+        # not if we do a "poetry update" or "poetry add".
+        expected_removals_count = 2 if is_installed and is_locked else 0
+
+    assert installer.executor.installations_count == expected_installations_count
+    assert installer.executor.removals_count == expected_removals_count
 
 
 @pytest.mark.network