build: ts-circular-deps tool should normalize golden (#36505)
Currently the golden output of the circular-deps tool is purely based on the order of source files passed to the tool, and on the amount of imports inside source files. This is actually resulting in deterministic output as running the tool multiple times without any changes to source files, results in the same output. Though it seems like the tool is too strict and we can avoid unnecessary golden changes if: 1. A source file that is part of a cycle is imported earlier (in terms of how the analyzer visits them). This could result in the cycle path starting with a different source file. 2. Source files which are not part of a cycle are imported earlier (in terms of how the analyzer visits them). This could result in moved items in the golden if re-approved (even though the cycles remain the same) To fix this, we normalize the cycle path array that serves as serializable data structure for the text-based goldens. Since the paths represents a cycle, the path can be shifted in a deterministic way so that cycles don't change unnecessarily in the golden, and to simplify comparison of cycles. Additionally, we sort the cycles in a deterministic way so that the golden doesn't change unnecessarily (as explained above). PR Close #36505
This commit is contained in:
parent
7d7b59efa5
commit
a8978ebf8e
|
@ -20,8 +20,17 @@ export type Golden = CircularDependency[];
|
|||
* the source file objects are mapped to their relative file names.
|
||||
*/
|
||||
export function convertReferenceChainToGolden(refs: ReferenceChain[], baseDir: string): Golden {
|
||||
return refs.map(
|
||||
chain => chain.map(({fileName}) => convertPathToForwardSlash(relative(baseDir, fileName))));
|
||||
return refs
|
||||
.map(
|
||||
// Normalize cycles as the paths can vary based on which node in the cycle is visited
|
||||
// first in the analyzer. The paths represent cycles. Hence we can shift nodes in a
|
||||
// deterministic way so that the goldens don't change unnecessarily and cycle comparison
|
||||
// is simpler.
|
||||
chain => normalizeCircularDependency(
|
||||
chain.map(({fileName}) => convertPathToForwardSlash(relative(baseDir, fileName)))))
|
||||
// Sort cycles so that the golden doesn't change unnecessarily when cycles are detected
|
||||
// in different order (e.g. new imports cause cycles to be detected earlier or later).
|
||||
.sort(compareCircularDependency);
|
||||
}
|
||||
|
||||
/**
|
||||
|
@ -44,6 +53,53 @@ export function compareGoldens(actual: Golden, expected: Golden) {
|
|||
return {newCircularDeps, fixedCircularDeps};
|
||||
}
|
||||
|
||||
/**
|
||||
* Normalizes the a circular dependency by ensuring that the path starts with the first
|
||||
* node in alphabetical order. Since the path array represents a cycle, we can make a
|
||||
* specific node the first element in the path that represents the cycle.
|
||||
*
|
||||
* This method is helpful because the path of circular dependencies changes based on which
|
||||
* file in the path has been visited first by the analyzer. e.g. Assume we have a circular
|
||||
* dependency represented as: `A -> B -> C`. The analyzer will detect this cycle when it
|
||||
* visits `A`. Though when a source file that is analyzed before `A` starts importing `B`,
|
||||
* the cycle path will detected as `B -> C -> A`. This represents the same cycle, but is just
|
||||
* different due to a limitation of using a data structure that can be written to a text-based
|
||||
* golden file.
|
||||
*
|
||||
* To account for this non-deterministic behavior in goldens, we shift the circular
|
||||
* dependency path to the first node based on alphabetical order. e.g. `A` will always
|
||||
* be the first node in the path that represents the cycle.
|
||||
*/
|
||||
function normalizeCircularDependency(path: CircularDependency): CircularDependency {
|
||||
if (path.length <= 1) {
|
||||
return path;
|
||||
}
|
||||
|
||||
let indexFirstNode: number = 0;
|
||||
let valueFirstNode: string = path[0];
|
||||
|
||||
// Find a node in the cycle path that precedes all other elements
|
||||
// in terms of alphabetical order.
|
||||
for (let i = 1; i < path.length; i++) {
|
||||
const value = path[i];
|
||||
if (value.localeCompare(valueFirstNode, 'en') < 0) {
|
||||
indexFirstNode = i;
|
||||
valueFirstNode = value;
|
||||
}
|
||||
}
|
||||
|
||||
// If the alphabetically first node is already at start of the path, just
|
||||
// return the actual path as no changes need to be made.
|
||||
if (indexFirstNode === 0) {
|
||||
return path;
|
||||
}
|
||||
|
||||
// Move the determined first node (as of alphabetical order) to the start of a new
|
||||
// path array. The nodes before the first node in the old path are then concatenated
|
||||
// to the end of the new path. This is possible because the path represents a cycle.
|
||||
return [...path.slice(indexFirstNode), ...path.slice(0, indexFirstNode)];
|
||||
}
|
||||
|
||||
/** Checks whether the specified circular dependencies are equal. */
|
||||
function isSameCircularDependency(actual: CircularDependency, expected: CircularDependency) {
|
||||
if (actual.length !== expected.length) {
|
||||
|
@ -56,3 +112,20 @@ function isSameCircularDependency(actual: CircularDependency, expected: Circular
|
|||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
/**
|
||||
* Compares two circular dependencies by respecting the alphabetic order of nodes in the
|
||||
* cycle paths. The first nodes which don't match in both paths are decisive on the order.
|
||||
*/
|
||||
function compareCircularDependency(a: CircularDependency, b: CircularDependency): number {
|
||||
// Go through nodes in both cycle paths and determine whether `a` should be ordered
|
||||
// before `b`. The first nodes which don't match decide on the order.
|
||||
for (let i = 0; i < Math.min(a.length, b.length); i++) {
|
||||
const compareValue = a[i].localeCompare(b[i], 'en');
|
||||
if (compareValue !== 0) {
|
||||
return compareValue;
|
||||
}
|
||||
}
|
||||
// If all nodes are equal in the cycles, the order is based on the length of both cycles.
|
||||
return a.length - b.length;
|
||||
}
|
||||
|
|
File diff suppressed because it is too large
Load Diff
Loading…
Reference in New Issue