diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 5c6dd94d..89e602f5 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -71,7 +71,7 @@ jobs: - name: Install uv and set the latest supported Python version uses: astral-sh/setup-uv@v7 with: - python-version: 3.13 + python-version: 3.14 - name: Update to a Cargo version that supports 2024 edition run: rustup install 1.90.0 && rustup default 1.90.0 diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 602aeabb..60c6699f 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -2,6 +2,11 @@ Changelog ========= +latest +------ + +* Add `nominate_cycle_breakers` method. + 3.12 (2025-10-09) ----------------- diff --git a/docs/usage.rst b/docs/usage.rst index dc7db5b2..1551b8cc 100644 --- a/docs/usage.rst +++ b/docs/usage.rst @@ -198,7 +198,7 @@ Methods for analysing direct imports Find all direct imports matching the passed import expression. - The imports are returned are in the following form:: + The imports returned are in the following form:: [ { @@ -505,6 +505,40 @@ Higher level analysis ``frozenset[str]``: Imported modules at the end of the chain. +.. py:function:: ImportGraph.nominate_cycle_breakers(package) + + Choose an approximately minimal set of dependencies that, if removed, would make the package locally acyclic. + + - 'Acyclic' means that there are no direct dependency cycles between the package's children. Indirect + dependencies (i.e. ones involving modules outside the supplied package) are disregarded, + as are imports between the package and its children. + - 'Dependency cycles' mean cycles between the *squashed* children (see `Terminology`_ above). + + Multiple sets of cycle breakers can exist for a given package. To arrive at this particular set, the following + approach is used: + + 1. Create a graph whose nodes are each child of the package. + 2. For each pair of children, add directed edges corresponding to whether there are imports between those two + children (as packages, rather than individual modules). The edges are weighted according to the number of + *dependencies* they represent: this is usually the same as the number of imports, but if a module imports + another module in multiple places, it will be treated as a single dependency. + 3. Calculate the approximate + `minimum weighted feedback arc set `_. + This attempts to find a set of edges with the smallest total weight that can be removed from the graph in order + to make it acyclic. It uses the greedy cycle-breaking heuristic of Eades, Lin and Smyth: not guaranteed + to find the optimal solution, but it is relatively fast. + 4. These edges are then used to look up all the concrete imports in the fully unsquashed graph, which are returned. + For example, an edge discovered in step 3. of ``mypackage.foo -> mypackage.bar``, with a weight 3, might correspond + to these three imports: ``mypackage.foo.blue -> mypackage.bar.green``, + ``mypackage.foo.blue.one -> mypackage.bar.green.two``, and ``mypackage.foo.blue -> mypackage.bar.green.three``. + + :param str package: The package in the graph to check for cycles. If a module with no children is passed, + an empty set is returned. + :return: A set of imports that, if removed, would make the imports between the the children of the supplied + package acyclic. + :rtype: ``set[tuple[str, str]]``. In each import tuple, the first element is the importer module and the second + is the imported. + Methods for manipulating the graph ---------------------------------- diff --git a/justfile b/justfile index 0de109f8..90debc13 100644 --- a/justfile +++ b/justfile @@ -64,6 +64,12 @@ test-python-3-13: test-python-3-14: UV_PYTHON=3.14 just test-python +# Populate missing Syrupy snapshots. +[group('testing')] +update-snapshots: + @uv run pytest --snapshot-update --benchmark-skip + + # Format the Rust code. [group('formatting')] [working-directory: 'rust'] diff --git a/pyproject.toml b/pyproject.toml index ace813b2..c8ab4101 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -67,7 +67,7 @@ docs = [ "sphinx-rtd-theme>=3.0.2", ] benchmark_ci = [ - "pytest-codspeed>=3.2.0", + "pytest-codspeed>=4.2.0", ] [tool.setuptools] diff --git a/rust/src/graph/cycle_breakers.rs b/rust/src/graph/cycle_breakers.rs new file mode 100644 index 00000000..3130d987 --- /dev/null +++ b/rust/src/graph/cycle_breakers.rs @@ -0,0 +1,249 @@ +use crate::errors::GrimpResult; + +use crate::graph::{Graph, ModuleToken}; +use rustc_hash::{FxHashMap, FxHashSet}; +use slotmap::SecondaryMap; +use std::cmp::Ordering; + +impl Graph { + pub fn nominate_cycle_breakers( + &self, + package: ModuleToken, + ) -> GrimpResult> { + // Get children of this package. + let children = match self.module_children.get(package) { + Some(children) => children, + None => return Ok(FxHashSet::default()), + }; + if children.len() < 2 { + return Ok(FxHashSet::default()); + } + let (orig_imports, orig_reverse_imports, edge_weights) = + self.build_graph_maps_of_children(children); + + // Make a copy of the graph. We'll iteratively remove nodes from this. + let mut working_imports = orig_imports.clone(); + let mut working_reverse_imports = orig_reverse_imports.clone(); + + // Iteratively extract sources into a vec. + let mut sources: Vec = vec![]; + + loop { + let current_sources: Vec = working_reverse_imports + .iter() + .filter(|(_imported, importers)| importers.is_empty()) + .map(|(source, _)| source) + .collect(); + + if current_sources.is_empty() { + break; + } + + sources.extend(current_sources); + // Remove sources from graph, then try again for another round. + for source in sources.iter() { + remove_module_from_graph( + source, + &mut working_imports, + &mut working_reverse_imports, + ); + } + } + + // Iteratively extract sinks into a vec. + let mut sinks: Vec = vec![]; + loop { + let new_sinks: Vec = working_imports + .iter() + .filter(|(_importer, importeds)| importeds.is_empty()) + .map(|(sink, _)| sink) + .collect(); + + if new_sinks.is_empty() { + break; + } + + // Remove sinks from graph, then try again for another round. + for sink in new_sinks.iter() { + remove_module_from_graph(sink, &mut working_imports, &mut working_reverse_imports); + } + // Add the new sinks to the beginning of sinks - these need to appear earlier in + // the overall order, as they depend on the sinks previously found. + sinks.splice(0..0, new_sinks); + //sinks.extend(new_sinks); + } + + // Iteratively extract remaining nodes, based on out-degree - in-degree. + let mut middle: Vec = vec![]; + loop { + if working_imports.is_empty() { + // We've finished ordering the nodes. + break; + } + + // Initialize with the first node. + let mut node_with_highest_difference: Option = None; + let mut highest_difference_so_far: Option = None; + + for (candidate, _) in working_imports.iter() { + let difference = calculate_degree_difference( + &candidate, + &working_imports, + &working_reverse_imports, + &edge_weights, + ); + + match node_with_highest_difference { + Some(incumbent) => { + match difference.cmp(&highest_difference_so_far.unwrap()) { + Ordering::Greater => { + highest_difference_so_far = Some(difference); + node_with_highest_difference = Some(candidate); + } + Ordering::Equal => { + // Tie breaker - choose the earlier one alphabetically. + let incumbent_name = self.get_module(incumbent).unwrap().name(); + let candidate_name = self.get_module(candidate).unwrap().name(); + + if candidate_name < incumbent_name { + highest_difference_so_far = Some(difference); + node_with_highest_difference = Some(candidate); + } + } + Ordering::Less => {} + } + } + None => { + node_with_highest_difference = Some(candidate); + highest_difference_so_far = Some(difference); + } + } + } + + // Extract node with highest difference to one of the two middle vecs. + let node_with_highest_difference = node_with_highest_difference.unwrap(); + + // Prioritize high out-degree. + middle.push(node_with_highest_difference); + remove_module_from_graph( + &node_with_highest_difference, + &mut working_imports, + &mut working_reverse_imports, + ); + } + if !working_reverse_imports.is_empty() { + panic!("Expected reverse imports also to be empty."); + } + + // Combine sources + ordered + sinks. + let full_ordering: Vec<_> = sources.into_iter().chain(middle).chain(sinks).collect(); + // Iterate through all edges in original graph. + // If the edge points leftwards in the full ordering, it is a circuit breaker. + let mut package_cycle_breakers: Vec<(ModuleToken, ModuleToken)> = vec![]; + for (importer, importeds) in orig_imports.iter() { + for imported in importeds.iter() { + let importer_position = get_module_position(&full_ordering, &importer).unwrap(); + let imported_position = get_module_position(&full_ordering, imported).unwrap(); + if imported_position < importer_position { + package_cycle_breakers.push((importer, *imported)); + } + } + } + + // Expand the package cycle breakers to the specific imports from the unsquashed graph. + let mut cycle_breakers = FxHashSet::default(); + for (importer, imported) in package_cycle_breakers { + cycle_breakers.extend( + self.find_direct_imports_between(importer, imported, true) + .unwrap(), + ); + } + + Ok(cycle_breakers) + } + + // These maps represent the local graph. Each module appears as a key in the maps, + // with empty sets if they have no imports to / from. + // They are denormalized into two maps for fast lookups. + #[allow(clippy::type_complexity)] + fn build_graph_maps_of_children( + &self, + children: &FxHashSet, + ) -> ( + SecondaryMap>, // Imports + SecondaryMap>, // Reverse imports + FxHashMap<(ModuleToken, ModuleToken), usize>, // Weights + ) { + let mut orig_imports: SecondaryMap> = + SecondaryMap::default(); + let mut orig_reverse_imports: SecondaryMap> = + SecondaryMap::default(); + let mut weights = FxHashMap::default(); + + for child in children { + orig_imports.insert(*child, FxHashSet::default()); + orig_reverse_imports.insert(*child, FxHashSet::default()); + } + // Get number of imports between each child. + // Add as edges. + for child_a in children { + for child_b in children.iter().filter(|child| *child != child_a) { + // Get number of imports from child_a to child_b (as package). + let num_imports_from_a_to_b = self + .find_direct_imports_between(*child_a, *child_b, true) + .unwrap() + .len(); + + if num_imports_from_a_to_b > 0 { + // a depends on b - add it as an edge in our temporary graph. + orig_imports[*child_a].insert(*child_b); + orig_reverse_imports[*child_b].insert(*child_a); + weights.insert((*child_a, *child_b), num_imports_from_a_to_b); + } + } + } + (orig_imports, orig_reverse_imports, weights) + } +} + +/// Removes the module from the graph, along with any imports to or from it. +fn remove_module_from_graph( + module: &ModuleToken, + imports: &mut SecondaryMap>, + reverse_imports: &mut SecondaryMap>, +) { + imports.remove(*module); + for (_, importeds) in imports.iter_mut() { + importeds.remove(module); + } + reverse_imports.remove(*module); + for (_, importers) in reverse_imports.iter_mut() { + importers.remove(module); + } +} + +/// Removes the module from the graph, along with any imports to or from it. +fn calculate_degree_difference( + module: &ModuleToken, + imports: &SecondaryMap>, + reverse_imports: &SecondaryMap>, + edge_weights: &FxHashMap<(ModuleToken, ModuleToken), usize>, +) -> isize { + let importer_modules = &reverse_imports[*module]; + let imported_modules = &imports[*module]; + + let indegree: isize = importer_modules + .iter() + .map(|importer| edge_weights[&(*importer, *module)] as isize) + .sum(); + let outdegree: isize = imported_modules + .iter() + .map(|imported| edge_weights[&(*module, *imported)] as isize) + .sum(); + + outdegree - indegree +} + +fn get_module_position(vec: &[ModuleToken], module: &ModuleToken) -> Option { + vec.iter().position(|&i| i == *module) +} diff --git a/rust/src/graph/direct_import_queries.rs b/rust/src/graph/direct_import_queries.rs index 3e9e9493..b3534d23 100644 --- a/rust/src/graph/direct_import_queries.rs +++ b/rust/src/graph/direct_import_queries.rs @@ -17,22 +17,54 @@ impl Graph { imported: ModuleToken, as_packages: bool, ) -> GrimpResult { - let mut importer: FxHashSet<_> = importer.into(); - let mut imported: FxHashSet<_> = imported.into(); + let mut importers: FxHashSet<_> = importer.into(); + let mut importeds: FxHashSet<_> = imported.into(); if as_packages { - importer.extend_with_descendants(self); - imported.extend_with_descendants(self); - if !(&importer & &imported).is_empty() { + importers.extend_with_descendants(self); + importeds.extend_with_descendants(self); + if !(&importers & &importeds).is_empty() { return Err(GrimpError::SharedDescendants); } } - let direct_imports = importer + let direct_imports = importers .iter() - .flat_map(|module| self.imports.get(*module).unwrap().iter().cloned()) + .flat_map(|importer_module| self.imports.get(*importer_module).unwrap().iter().cloned()) .collect::>(); - Ok(!(&direct_imports & &imported).is_empty()) + Ok(!(&direct_imports & &importeds).is_empty()) + } + + pub fn find_direct_imports_between( + &self, + importer: ModuleToken, + imported: ModuleToken, + as_packages: bool, + ) -> GrimpResult> { + let mut all_imports: FxHashSet<(ModuleToken, ModuleToken)> = FxHashSet::default(); + + let mut importers: FxHashSet<_> = importer.into(); + let mut importeds: FxHashSet<_> = imported.into(); + if as_packages { + importers.extend_with_descendants(self); + importeds.extend_with_descendants(self); + if !(&importers & &importeds).is_empty() { + return Err(GrimpError::SharedDescendants); + } + } + + for importer_module in importers.iter() { + if let Some(all_imports_imported_by_this_one) = self.imports.get(*importer_module) { + let imports_of_supplied_package: FxHashSet<_> = all_imports_imported_by_this_one + .iter() + .filter(|candidate| importeds.contains(*candidate)) + .map(|imported_module| (*importer_module, *imported_module)) + .collect(); + all_imports.extend(imports_of_supplied_package); + } + } + + Ok(all_imports) } pub fn modules_directly_imported_by(&self, importer: ModuleToken) -> &FxHashSet { diff --git a/rust/src/graph/mod.rs b/rust/src/graph/mod.rs index 934528ec..b01e82b7 100644 --- a/rust/src/graph/mod.rs +++ b/rust/src/graph/mod.rs @@ -25,6 +25,7 @@ pub mod hierarchy_queries; pub mod higher_order_queries; pub mod import_chain_queries; +pub mod cycle_breakers; pub(crate) mod pathfinding; static MODULE_NAMES: LazyLock>> = @@ -619,6 +620,35 @@ impl GraphWrapper { self.convert_package_dependencies_to_python(py, illegal_dependencies) } + pub fn nominate_cycle_breakers<'py>( + &self, + py: Python<'py>, + package: &str, + ) -> PyResult> { + let package = self.get_visible_module_by_name(package)?.token(); + let cycle_breakers = self._graph.nominate_cycle_breakers(package)?; + PySet::new( + py, + cycle_breakers + .into_iter() + .map(|(importer, imported)| { + let importer = self._graph.get_module(importer).unwrap(); + let imported = self._graph.get_module(imported).unwrap(); + Import::new(importer.name(), imported.name()) + }) + .map(|import| { + PyTuple::new( + py, + [ + import.importer.into_py_any(py).unwrap(), + import.imported.into_py_any(py).unwrap(), + ], + ) + .unwrap() + }), + ) + } + #[pyo3(name = "clone")] pub fn clone_py(&self) -> GraphWrapper { self.clone() diff --git a/src/grimp/application/graph.py b/src/grimp/application/graph.py index f9487e6f..4c31f52b 100644 --- a/src/grimp/application/graph.py +++ b/src/grimp/application/graph.py @@ -2,7 +2,6 @@ from typing import List, Optional, Sequence, Set, Tuple, TypedDict from grimp.domain.analysis import PackageDependency, Route from grimp.domain.valueobjects import Layer - from grimp import _rustgrimp as rust # type: ignore[attr-defined] from grimp.exceptions import ( ModuleNotPresent, @@ -17,6 +16,11 @@ class Import(TypedDict): imported: str +# Corresponds to importer, imported. +# Prefer this form to Import, as it's both more lightweight, and hashable. +ImportTuple = Tuple[str, str] + + class DetailedImport(Import): line_number: int line_contents: str @@ -440,6 +444,14 @@ def find_illegal_dependencies_for_layers( return _dependencies_from_tuple(result) + def nominate_cycle_breakers(self, package: str) -> set[ImportTuple]: + """ + Identify a set of imports that, if removed, would make the package locally acyclic. + """ + if not self._rustgraph.contains_module(package): + raise ModuleNotPresent(f'"{package}" not present in the graph.') + return self._rustgraph.nominate_cycle_breakers(package) + # Dunder methods # -------------- diff --git a/tests/benchmarking/test_benchmarking.py b/tests/benchmarking/test_benchmarking.py index 5e5b4517..aece8dad 100644 --- a/tests/benchmarking/test_benchmarking.py +++ b/tests/benchmarking/test_benchmarking.py @@ -571,3 +571,17 @@ def test_find_matching_direct_imports(benchmark, large_graph): ), ) assert len(matching_imports) == 4051 + + +def test_nominate_cycle_breakers_django(benchmark): + graph = grimp.build_graph("django") + + benchmark(graph.nominate_cycle_breakers, "django") + + +def test_nominate_cycle_breakers_large_graph_root(benchmark, large_graph): + benchmark(large_graph.nominate_cycle_breakers, "mypackage") + + +def test_nominate_cycle_breakers_large_graph_subpackage(benchmark, large_graph): + benchmark(large_graph.nominate_cycle_breakers, "mypackage.domain") diff --git a/tests/functional/__snapshots__/test_build_graph_on_real_packages.ambr b/tests/functional/__snapshots__/test_build_graph_on_real_packages.ambr index d8ad0173..d0e139a9 100644 --- a/tests/functional/__snapshots__/test_build_graph_on_real_packages.ambr +++ b/tests/functional/__snapshots__/test_build_graph_on_real_packages.ambr @@ -21455,3 +21455,931 @@ }), ]) # --- +# name: test_nominate_cycle_breakers_django[django.db.models] + set({ + tuple( + 'django.db.models.aggregates', + 'django.db.models.fields', + ), + tuple( + 'django.db.models.aggregates', + 'django.db.models.functions.comparison', + ), + tuple( + 'django.db.models.aggregates', + 'django.db.models.functions.mixins', + ), + tuple( + 'django.db.models.deletion', + 'django.db.models.sql', + ), + tuple( + 'django.db.models.expressions', + 'django.db.models.fields', + ), + tuple( + 'django.db.models.fields.related', + 'django.db.models.base', + ), + tuple( + 'django.db.models.fields.related_descriptors', + 'django.db.models.functions', + ), + tuple( + 'django.db.models.indexes', + 'django.db.models.functions', + ), + tuple( + 'django.db.models.indexes', + 'django.db.models.sql', + ), + tuple( + 'django.db.models.lookups', + 'django.db.models.fields', + ), + tuple( + 'django.db.models.lookups', + 'django.db.models.functions', + ), + tuple( + 'django.db.models.lookups', + 'django.db.models.sql.query', + ), + tuple( + 'django.db.models.manager', + 'django.db.models.query', + ), + tuple( + 'django.db.models.query', + 'django.db.models.functions', + ), + tuple( + 'django.db.models.query_utils', + 'django.db.models.functions', + ), + tuple( + 'django.db.models.query_utils', + 'django.db.models.lookups', + ), + tuple( + 'django.db.models.query_utils', + 'django.db.models.sql', + ), + tuple( + 'django.db.models.query_utils', + 'django.db.models.sql.constants', + ), + tuple( + 'django.db.models.sql.compiler', + 'django.db.models.functions', + ), + tuple( + 'django.db.models.sql.query', + 'django.db.models.fields', + ), + tuple( + 'django.db.models.sql.query', + 'django.db.models.fields.related_lookups', + ), + }) +# --- +# name: test_nominate_cycle_breakers_django[django.db] + set({ + tuple( + 'django.db.models.fields.related', + 'django.db.backends.utils', + ), + tuple( + 'django.db.models.functions.mixins', + 'django.db.backends.oracle.functions', + ), + tuple( + 'django.db.models.indexes', + 'django.db.backends.utils', + ), + tuple( + 'django.db.models.options', + 'django.db.backends.utils', + ), + tuple( + 'django.db.utils', + 'django.db.backends', + ), + }) +# --- +# name: test_nominate_cycle_breakers_django[django] + set({ + tuple( + 'django.apps.config', + 'django.core.exceptions', + ), + tuple( + 'django.apps.config', + 'django.utils.functional', + ), + tuple( + 'django.apps.config', + 'django.utils.module_loading', + ), + tuple( + 'django.apps.registry', + 'django.core.exceptions', + ), + tuple( + 'django.conf', + 'django.core.exceptions', + ), + tuple( + 'django.conf', + 'django.utils.deprecation', + ), + tuple( + 'django.conf', + 'django.utils.functional', + ), + tuple( + 'django.conf.urls', + 'django.views.defaults', + ), + tuple( + 'django.conf.urls.i18n', + 'django.views.i18n', + ), + tuple( + 'django.conf.urls.static', + 'django.core.exceptions', + ), + tuple( + 'django.conf.urls.static', + 'django.views.static', + ), + tuple( + 'django.core.cache.backends.db', + 'django.db', + ), + tuple( + 'django.core.cache.backends.db', + 'django.db.models', + ), + tuple( + 'django.core.cache.backends.db', + 'django.db.transaction', + ), + tuple( + 'django.core.checks.database', + 'django.db', + ), + tuple( + 'django.core.checks.messages', + 'django.db.models', + ), + tuple( + 'django.core.checks.model_checks', + 'django.db.models.signals', + ), + tuple( + 'django.core.handlers.base', + 'django.db', + ), + tuple( + 'django.core.handlers.base', + 'django.db.transaction', + ), + tuple( + 'django.core.management.base', + 'django.db', + ), + tuple( + 'django.core.management.base', + 'django.db.migrations.executor', + ), + tuple( + 'django.core.management.commands.createcachetable', + 'django.db', + ), + tuple( + 'django.core.management.commands.createcachetable', + 'django.db.models', + ), + tuple( + 'django.core.management.commands.createcachetable', + 'django.db.transaction', + ), + tuple( + 'django.core.management.commands.dbshell', + 'django.db', + ), + tuple( + 'django.core.management.commands.dumpdata', + 'django.db', + ), + tuple( + 'django.core.management.commands.flush', + 'django.db', + ), + tuple( + 'django.core.management.commands.inspectdb', + 'django.db', + ), + tuple( + 'django.core.management.commands.inspectdb', + 'django.db.models.constants', + ), + tuple( + 'django.core.management.commands.loaddata', + 'django.db', + ), + tuple( + 'django.core.management.commands.loaddata', + 'django.db.transaction', + ), + tuple( + 'django.core.management.commands.makemigrations', + 'django.db', + ), + tuple( + 'django.core.management.commands.makemigrations', + 'django.db.migrations', + ), + tuple( + 'django.core.management.commands.makemigrations', + 'django.db.migrations.autodetector', + ), + tuple( + 'django.core.management.commands.makemigrations', + 'django.db.migrations.loader', + ), + tuple( + 'django.core.management.commands.makemigrations', + 'django.db.migrations.migration', + ), + tuple( + 'django.core.management.commands.makemigrations', + 'django.db.migrations.optimizer', + ), + tuple( + 'django.core.management.commands.makemigrations', + 'django.db.migrations.questioner', + ), + tuple( + 'django.core.management.commands.makemigrations', + 'django.db.migrations.state', + ), + tuple( + 'django.core.management.commands.makemigrations', + 'django.db.migrations.utils', + ), + tuple( + 'django.core.management.commands.makemigrations', + 'django.db.migrations.writer', + ), + tuple( + 'django.core.management.commands.migrate', + 'django.db', + ), + tuple( + 'django.core.management.commands.migrate', + 'django.db.migrations.autodetector', + ), + tuple( + 'django.core.management.commands.migrate', + 'django.db.migrations.executor', + ), + tuple( + 'django.core.management.commands.migrate', + 'django.db.migrations.loader', + ), + tuple( + 'django.core.management.commands.migrate', + 'django.db.migrations.state', + ), + tuple( + 'django.core.management.commands.optimizemigration', + 'django.db.migrations', + ), + tuple( + 'django.core.management.commands.optimizemigration', + 'django.db.migrations.exceptions', + ), + tuple( + 'django.core.management.commands.optimizemigration', + 'django.db.migrations.loader', + ), + tuple( + 'django.core.management.commands.optimizemigration', + 'django.db.migrations.optimizer', + ), + tuple( + 'django.core.management.commands.optimizemigration', + 'django.db.migrations.writer', + ), + tuple( + 'django.core.management.commands.showmigrations', + 'django.db', + ), + tuple( + 'django.core.management.commands.showmigrations', + 'django.db.migrations.loader', + ), + tuple( + 'django.core.management.commands.showmigrations', + 'django.db.migrations.recorder', + ), + tuple( + 'django.core.management.commands.sqlflush', + 'django.db', + ), + tuple( + 'django.core.management.commands.sqlmigrate', + 'django.db', + ), + tuple( + 'django.core.management.commands.sqlmigrate', + 'django.db.migrations.loader', + ), + tuple( + 'django.core.management.commands.sqlsequencereset', + 'django.db', + ), + tuple( + 'django.core.management.commands.squashmigrations', + 'django.db', + ), + tuple( + 'django.core.management.commands.squashmigrations', + 'django.db.migrations', + ), + tuple( + 'django.core.management.commands.squashmigrations', + 'django.db.migrations.loader', + ), + tuple( + 'django.core.management.commands.squashmigrations', + 'django.db.migrations.migration', + ), + tuple( + 'django.core.management.commands.squashmigrations', + 'django.db.migrations.optimizer', + ), + tuple( + 'django.core.management.commands.squashmigrations', + 'django.db.migrations.writer', + ), + tuple( + 'django.core.management.commands.testserver', + 'django.db', + ), + tuple( + 'django.core.management.sql', + 'django.db.models', + ), + tuple( + 'django.core.serializers.base', + 'django.db.models', + ), + tuple( + 'django.core.serializers.python', + 'django.db', + ), + tuple( + 'django.core.serializers.python', + 'django.db.models', + ), + tuple( + 'django.core.serializers.pyyaml', + 'django.db.models', + ), + tuple( + 'django.core.serializers.xml_serializer', + 'django.db', + ), + tuple( + 'django.core.serializers.xml_serializer', + 'django.db.models', + ), + tuple( + 'django.core.servers.basehttp', + 'django.db', + ), + tuple( + 'django.dispatch.dispatcher', + 'django.conf', + ), + tuple( + 'django.dispatch.dispatcher', + 'django.utils.inspect', + ), + tuple( + 'django.forms', + 'django.core.exceptions', + ), + tuple( + 'django.forms.boundfield', + 'django.core.exceptions', + ), + tuple( + 'django.forms.fields', + 'django.core.exceptions', + ), + tuple( + 'django.forms.fields', + 'django.core.validators', + ), + tuple( + 'django.forms.forms', + 'django.core.exceptions', + ), + tuple( + 'django.forms.formsets', + 'django.core.exceptions', + ), + tuple( + 'django.forms.models', + 'django.core.exceptions', + ), + tuple( + 'django.forms.models', + 'django.db.models', + ), + tuple( + 'django.forms.models', + 'django.db.models.utils', + ), + tuple( + 'django.forms.utils', + 'django.core.exceptions', + ), + tuple( + 'django.http.multipartparser', + 'django.core.exceptions', + ), + tuple( + 'django.http.multipartparser', + 'django.core.files.uploadhandler', + ), + tuple( + 'django.http.request', + 'django.core.exceptions', + ), + tuple( + 'django.http.request', + 'django.core.files.uploadhandler', + ), + tuple( + 'django.http.request', + 'django.core.signing', + ), + tuple( + 'django.http.response', + 'django.core.exceptions', + ), + tuple( + 'django.http.response', + 'django.core.serializers.json', + ), + tuple( + 'django.http.response', + 'django.core.signals', + ), + tuple( + 'django.http.response', + 'django.core.signing', + ), + tuple( + 'django.middleware.cache', + 'django.core.cache', + ), + tuple( + 'django.middleware.common', + 'django.core.exceptions', + ), + tuple( + 'django.middleware.common', + 'django.core.mail', + ), + tuple( + 'django.middleware.csrf', + 'django.core.exceptions', + ), + tuple( + 'django.shortcuts', + 'django.http', + ), + tuple( + 'django.shortcuts', + 'django.template.loader', + ), + tuple( + 'django.shortcuts', + 'django.utils.functional', + ), + tuple( + 'django.template.backends.base', + 'django.core.exceptions', + ), + tuple( + 'django.template.backends.dummy', + 'django.core.exceptions', + ), + tuple( + 'django.template.context_processors', + 'django.db', + ), + tuple( + 'django.template.engine', + 'django.core.exceptions', + ), + tuple( + 'django.template.loaders.filesystem', + 'django.core.exceptions', + ), + tuple( + 'django.template.utils', + 'django.core.exceptions', + ), + tuple( + 'django.templatetags.cache', + 'django.core.cache', + ), + tuple( + 'django.templatetags.cache', + 'django.core.cache.utils', + ), + tuple( + 'django.templatetags.cache', + 'django.template', + ), + tuple( + 'django.templatetags.i18n', + 'django.template', + ), + tuple( + 'django.templatetags.i18n', + 'django.template.base', + ), + tuple( + 'django.templatetags.i18n', + 'django.template.defaulttags', + ), + tuple( + 'django.templatetags.l10n', + 'django.template', + ), + tuple( + 'django.templatetags.static', + 'django.contrib.staticfiles.storage', + ), + tuple( + 'django.templatetags.static', + 'django.template', + ), + tuple( + 'django.templatetags.tz', + 'django.template', + ), + tuple( + 'django.test.client', + 'django.contrib.auth', + ), + tuple( + 'django.test.client', + 'django.core.handlers.asgi', + ), + tuple( + 'django.test.client', + 'django.core.handlers.base', + ), + tuple( + 'django.test.client', + 'django.core.handlers.wsgi', + ), + tuple( + 'django.test.client', + 'django.core.serializers.json', + ), + tuple( + 'django.test.client', + 'django.core.signals', + ), + tuple( + 'django.test.client', + 'django.db', + ), + tuple( + 'django.test.runner', + 'django.core.management', + ), + tuple( + 'django.test.runner', + 'django.db', + ), + tuple( + 'django.test.signals', + 'django.contrib.auth', + ), + tuple( + 'django.test.signals', + 'django.contrib.auth.backends', + ), + tuple( + 'django.test.signals', + 'django.contrib.auth.forms', + ), + tuple( + 'django.test.signals', + 'django.contrib.auth.handlers.modwsgi', + ), + tuple( + 'django.test.signals', + 'django.contrib.auth.management.commands.changepassword', + ), + tuple( + 'django.test.signals', + 'django.contrib.auth.password_validation', + ), + tuple( + 'django.test.signals', + 'django.contrib.auth.views', + ), + tuple( + 'django.test.signals', + 'django.contrib.staticfiles.finders', + ), + tuple( + 'django.test.signals', + 'django.contrib.staticfiles.storage', + ), + tuple( + 'django.test.signals', + 'django.core.cache', + ), + tuple( + 'django.test.signals', + 'django.core.exceptions', + ), + tuple( + 'django.test.signals', + 'django.core.files.storage', + ), + tuple( + 'django.test.signals', + 'django.core.management', + ), + tuple( + 'django.test.signals', + 'django.core.serializers', + ), + tuple( + 'django.test.signals', + 'django.core.signals', + ), + tuple( + 'django.test.signals', + 'django.db', + ), + tuple( + 'django.test.signals', + 'django.db.utils', + ), + tuple( + 'django.test.testcases', + 'django.core.exceptions', + ), + tuple( + 'django.test.testcases', + 'django.core.files.locks', + ), + tuple( + 'django.test.testcases', + 'django.core.handlers.wsgi', + ), + tuple( + 'django.test.testcases', + 'django.core.mail', + ), + tuple( + 'django.test.testcases', + 'django.core.management', + ), + tuple( + 'django.test.testcases', + 'django.core.management.color', + ), + tuple( + 'django.test.testcases', + 'django.core.management.sql', + ), + tuple( + 'django.test.testcases', + 'django.core.servers.basehttp', + ), + tuple( + 'django.test.testcases', + 'django.core.signals', + ), + tuple( + 'django.test.testcases', + 'django.db', + ), + tuple( + 'django.test.testcases', + 'django.db.transaction', + ), + tuple( + 'django.test.testcases', + 'django.views.static', + ), + tuple( + 'django.test.utils', + 'django.core.checks.registry', + ), + tuple( + 'django.test.utils', + 'django.core.exceptions', + ), + tuple( + 'django.test.utils', + 'django.core.mail', + ), + tuple( + 'django.test.utils', + 'django.core.signals', + ), + tuple( + 'django.test.utils', + 'django.db', + ), + tuple( + 'django.test.utils', + 'django.db.models.options', + ), + tuple( + 'django.urls.base', + 'django.utils.functional', + ), + tuple( + 'django.urls.base', + 'django.utils.translation', + ), + tuple( + 'django.urls.conf', + 'django.core.exceptions', + ), + tuple( + 'django.urls.conf', + 'django.views', + ), + tuple( + 'django.urls.exceptions', + 'django.http', + ), + tuple( + 'django.urls.resolvers', + 'django.conf', + ), + tuple( + 'django.urls.resolvers', + 'django.conf.urls', + ), + tuple( + 'django.urls.resolvers', + 'django.core.checks', + ), + tuple( + 'django.urls.resolvers', + 'django.core.checks.urls', + ), + tuple( + 'django.urls.resolvers', + 'django.core.exceptions', + ), + tuple( + 'django.urls.resolvers', + 'django.utils.datastructures', + ), + tuple( + 'django.urls.resolvers', + 'django.utils.functional', + ), + tuple( + 'django.urls.resolvers', + 'django.utils.http', + ), + tuple( + 'django.urls.resolvers', + 'django.utils.regex_helper', + ), + tuple( + 'django.urls.resolvers', + 'django.utils.translation', + ), + tuple( + 'django.urls.resolvers', + 'django.views', + ), + tuple( + 'django.urls.utils', + 'django.core.exceptions', + ), + tuple( + 'django.urls.utils', + 'django.utils.module_loading', + ), + tuple( + 'django.utils._os', + 'django.core.exceptions', + ), + tuple( + 'django.utils.archive', + 'django.core.exceptions', + ), + tuple( + 'django.utils.asyncio', + 'django.core.exceptions', + ), + tuple( + 'django.utils.autoreload', + 'django.core.signals', + ), + tuple( + 'django.utils.cache', + 'django.core.cache', + ), + tuple( + 'django.utils.cache', + 'django.http', + ), + tuple( + 'django.utils.html', + 'django.core.exceptions', + ), + tuple( + 'django.utils.html', + 'django.core.serializers.json', + ), + tuple( + 'django.utils.ipv6', + 'django.core.exceptions', + ), + tuple( + 'django.utils.log', + 'django.core.mail', + ), + tuple( + 'django.utils.log', + 'django.core.management.color', + ), + tuple( + 'django.utils.text', + 'django.core.exceptions', + ), + tuple( + 'django.utils.translation.template', + 'django.template.base', + ), + tuple( + 'django.utils.translation.trans_real', + 'django.core.exceptions', + ), + tuple( + 'django.utils.translation.trans_real', + 'django.core.signals', + ), + tuple( + 'django.views.generic.base', + 'django.core.exceptions', + ), + tuple( + 'django.views.generic.dates', + 'django.core.exceptions', + ), + tuple( + 'django.views.generic.dates', + 'django.db.models', + ), + tuple( + 'django.views.generic.detail', + 'django.core.exceptions', + ), + tuple( + 'django.views.generic.detail', + 'django.db.models', + ), + tuple( + 'django.views.generic.edit', + 'django.core.exceptions', + ), + tuple( + 'django.views.generic.list', + 'django.core.exceptions', + ), + tuple( + 'django.views.generic.list', + 'django.core.paginator', + ), + tuple( + 'django.views.generic.list', + 'django.db.models', + ), + }) +# --- diff --git a/tests/functional/test_build_graph_on_real_packages.py b/tests/functional/test_build_graph_on_real_packages.py index 3746d005..b2da12af 100644 --- a/tests/functional/test_build_graph_on_real_packages.py +++ b/tests/functional/test_build_graph_on_real_packages.py @@ -11,3 +11,15 @@ def test_build_graph_on_real_package(package_name, snapshot): graph = grimp.build_graph(package_name, cache_dir=None) imports = graph.find_matching_direct_imports(import_expression="** -> **") assert imports == snapshot + + +@pytest.mark.parametrize( + "package_name", + ("django", "django.db", "django.db.models"), +) +def test_nominate_cycle_breakers_django(package_name, snapshot): + graph = grimp.build_graph("django") + + cycle_breakers = graph.nominate_cycle_breakers(package_name) + + assert cycle_breakers == snapshot diff --git a/tests/unit/application/graph/test_cycle_breakers.py b/tests/unit/application/graph/test_cycle_breakers.py new file mode 100644 index 00000000..7e78881d --- /dev/null +++ b/tests/unit/application/graph/test_cycle_breakers.py @@ -0,0 +1,235 @@ +from grimp.application.graph import ImportGraph +import pytest +from grimp.exceptions import ModuleNotPresent + + +class TestNominateCycleBreakers: + def test_empty_graph(self): + graph = ImportGraph() + graph.add_module("pkg") + + result = graph.nominate_cycle_breakers("pkg") + + assert result == set() + + def test_nonexistent_module(self): + graph = ImportGraph() + graph.add_module("pkg") + + with pytest.raises(ModuleNotPresent): + graph.nominate_cycle_breakers("nonexistent") + + @pytest.mark.parametrize( + "module", + ( + "pkg", + "pkg.foo", + "pkg.foo.blue", + ), + ) + def test_graph_with_no_imports(self, module: str): + graph = self._build_graph_with_no_imports() + + result = graph.nominate_cycle_breakers(module) + + assert result == set() + + @pytest.mark.parametrize( + "module", + ( + "pkg", + "pkg.bar", + "pkg.foo.blue", + "pkg.foo.green", # Leaf package. + ), + ) + def test_acyclic_graph(self, module: str): + graph = self._build_acyclic_graph() + + result = graph.nominate_cycle_breakers(module) + + assert result == set() + + def test_one_breaker(self): + graph = self._build_acyclic_graph() + importer, imported = "pkg.bar.red.four", "pkg.foo.blue.two" + graph.add_import(importer=importer, imported=imported) + result = graph.nominate_cycle_breakers("pkg") + + assert result == {(importer, imported)} + + def test_three_breakers(self): + graph = self._build_acyclic_graph() + imports = { + ("pkg.bar.red.four", "pkg.foo.blue.two"), + ("pkg.bar.yellow", "pkg.foo.blue.three"), + ("pkg.bar", "pkg.foo.blue.three"), + } + for importer, imported in imports: + graph.add_import(importer=importer, imported=imported) + + result = graph.nominate_cycle_breakers("pkg") + + assert result == imports + + def test_nominated_based_on_dependencies_rather_than_imports(self): + graph = self._build_acyclic_graph() + # Add lots of imports from a single module - this will be treated as + # a single dependency. + importer, imported = "pkg.bar.red.four", "pkg.foo.blue.two" + for i in range(1, 30): + graph.add_import( + importer=importer, imported=imported, line_number=i, line_contents="-" + ) + + graph.add_import(importer=importer, imported=imported) + + result = graph.nominate_cycle_breakers("pkg") + + assert result == {(importer, imported)} + + def test_imports_between_passed_package_and_children_are_disregarded(self): + graph = self._build_acyclic_graph() + parent, child = "pkg.foo.blue", "pkg.foo" + graph.add_import(importer=parent, imported=child) + graph.add_import(importer=child, imported=parent) + + result = graph.nominate_cycle_breakers(parent) + + assert result == set() + + def test_on_child_of_root(self): + graph = self._build_acyclic_graph() + imports = { + ("pkg.bar.red.five", "pkg.bar.yellow.eight"), + ("pkg.bar.red", "pkg.bar.yellow"), + } + for importer, imported in imports: + graph.add_import(importer=importer, imported=imported) + + result = graph.nominate_cycle_breakers("pkg.bar") + + assert result == imports + + def test_on_grandchild_of_root(self): + graph = self._build_acyclic_graph() + imports = { + ("pkg.bar.orange.ten.gamma", "pkg.bar.orange.nine.alpha"), + ("pkg.bar.orange.ten", "pkg.bar.orange.nine.alpha"), + } + for importer, imported in imports: + graph.add_import(importer=importer, imported=imported) + + result = graph.nominate_cycle_breakers("pkg.bar.orange") + + assert result == imports + + def test_on_package_with_one_child(self): + graph = self._build_acyclic_graph() + graph.add_module("pkg.bar.orange.ten.gamma.onechild") + + result = graph.nominate_cycle_breakers("pkg.bar.orange.ten.gamma") + + assert result == set() + + @pytest.mark.parametrize("order", (sorted, reversed)) + def test_tie_breaker_orders_nodes_alphabetically(self, order): + graph = ImportGraph() + graph.add_module("pkg") + # Add a -> b -> c -> d -> e -> a in a loop. + # We try both orderings to check it's independent of the order + # in which they're added. + for importer, imported in order( + ( + ("a", "b"), + ("b", "c"), + ("c", "d"), + ("d", "e"), + ("e", "f"), + ("f", "a"), + ) + ): + graph.add_import(importer=f"pkg.{importer}", imported=f"pkg.{imported}") + + result = graph.nominate_cycle_breakers("pkg") + + # The tie-breaker for selecting nodes to order is alphabetical. + # That means an import from a later item in the alphabet will be selected first. + assert result == {("pkg.f", "pkg.a")} + + def test_works_with_chains_of_sources_and_sinks(self): + graph = ImportGraph() + graph.add_module("pkg") + for importer, imported in ( + # Sources + ("a", "b"), + ("b", "c"), + ("c", "d"), + # Cycle + ("d", "e"), + ("e", "f"), + ("f", "d"), + # Sinks + ("f", "g"), + ("g", "h"), + ("h", "i"), + ): + graph.add_import(importer=f"pkg.{importer}", imported=f"pkg.{imported}") + + result = graph.nominate_cycle_breakers("pkg") + + assert result == {("pkg.f", "pkg.d")} + + def _build_graph_with_no_imports(self) -> ImportGraph: + graph = ImportGraph() + for module in ( + "pkg", + "pkg.foo", + "pkg.foo.blue", + "pkg.foo.blue.one", + "pkg.foo.blue.two", + "pkg.foo.green", + "pkg.bar", + "pkg.bar.red", + "pkg.bar.red.three", + "pkg.bar.red.four", + "pkg.bar.red.five", + "pkg.bar.red.six", + "pkg.bar.red.seven", + "pkg.bar.yellow", + "pkg.bar.yellow.eight", + "pkg.bar.orange", + "pkg.bar.orange.nine", + "pkg.bar.orange.nine.alpha", + "pkg.bar.orange.nine.beta", + "pkg.bar.orange.ten", + "pkg.bar.orange.ten.gamma", + "pkg.bar.orange.ten.delta", + ): + graph.add_module(module) + return graph + + def _build_acyclic_graph(self) -> ImportGraph: + graph = self._build_graph_with_no_imports() + # Add imports that make: + # pkg.foo -> pkg.bar + # pkg.bar.yellow -> pkg.foo.red + # pkg.bar.orange.nine -> pkg.bar.orange.ten + for importer, imported in ( + ("pkg.foo", "pkg.bar.red"), + ("pkg.foo.green", "pkg.bar.yellow"), + ("pkg.foo.blue.two", "pkg.bar.red.three"), + ("pkg.foo.blue.two", "pkg.bar.red.four"), + ("pkg.foo.blue.two", "pkg.bar.red.five"), + ("pkg.foo.blue.two", "pkg.bar.red.six"), + ("pkg.foo.blue.two", "pkg.bar.red.seven"), + ("pkg.bar.yellow", "pkg.bar.red"), + ("pkg.bar.yellow.eight", "pkg.bar.red.three"), + ("pkg.bar.yellow.eight", "pkg.bar.red.four"), + ("pkg.bar.yellow.eight", "pkg.bar.red.five"), + ("pkg.bar.orange.nine", "pkg.bar.orange.ten.gamma"), + ("pkg.bar.orange.nine.alpha", "pkg.bar.orange.ten.gamma"), + ("pkg.bar.orange.nine.beta", "pkg.bar.orange.ten.delta"), + ): + graph.add_import(importer=importer, imported=imported) + return graph