Detect package names added/removed from attw.json as changed (#944)

* Detect package names added/removed from attw.json as changed

* Add changeset

* Only run npm checks when attw.json is changed

* Fix changeset

.changeset/weak-flowers-fail.md

@@ -0,0 +1,8 @@
+---
+"@definitelytyped/definitions-parser": patch
+"@definitelytyped/dtslint": patch
+"@definitelytyped/dtslint-runner": patch
+"@definitelytyped/utils": patch
+---
+
+Detect package names added/removed from attw.json as changed

packages/definitions-parser/src/get-affected-packages.ts

@@ -5,12 +5,13 @@ import { satisfies } from "semver";
 export interface PreparePackagesResult {
   readonly packageNames: Set<string>;
   readonly dependents: Set<string>;
+  readonly attwChanges: Set<string>;
 }
 
 /** Gets all packages that have changed on this branch, plus all packages affected by the change. */
 export async function getAffectedPackages(
   allPackages: AllPackages,
-  git: { deletions: PackageId[]; additions: PackageId[] },
+  git: { deletions: PackageId[]; additions: PackageId[]; attwChanges: PackageId[] },
   definitelyTypedPath: string,
   diffBase: string,
 ): Promise<PreparePackagesResult> {
@@ -62,6 +63,7 @@ export async function getAffectedPackages(
     allPackages,
     changedPackageDirectories,
     addedPackageDirectories,
+    git.attwChanges,
     allDependentDirectories,
     definitelyTypedPath,
   );
@@ -71,6 +73,7 @@ export async function getAffectedPackagesWorker(
   allPackages: AllPackages,
   changedOutput: string,
   additions: string[],
+  attwChangedPackages: PackageId[],
   dependentOutputs: string[],
   definitelyTypedPath: string,
 ): Promise<PreparePackagesResult> {
@@ -84,7 +87,16 @@ export async function getAffectedPackagesWorker(
   const dependents = new Set(
     (await Promise.all(dependentDirs.map(tryGetTypingsData))).filter((d): d is string => !!d && !packageNames.has(d)),
   );
-  return { packageNames, dependents };
+  const attwChanges = new Set(
+    (
+      await Promise.all(attwChangedPackages.map(async (id) => (await allPackages.tryGetTypingsData(id))?.subDirectoryPath))
+    ).filter((d): d is string => !!d && !packageNames.has(d) && !dependents.has(d)),
+  );
+  return {
+    packageNames,
+    dependents,
+    attwChanges,
+  };
 
   async function tryGetTypingsData(d: string) {
     const dep = getDependencyFromFile(normalizeSlashes(d + "/index.d.ts"));

packages/definitions-parser/src/git.ts

@@ -1,9 +1,11 @@
-import { PackageId, AllPackages, NotNeededPackage, getDependencyFromFile, formatTypingVersion } from "./packages";
-import { Logger, execAndThrowErrors, consoleLogger, assertDefined, cacheDir } from "@definitelytyped/utils";
+import { Logger, assertDefined, cacheDir, consoleLogger, execAndThrowErrors, joinPaths, symmetricDifference } from "@definitelytyped/utils";
 import * as pacote from "pacote";
 import * as semver from "semver";
 import { inspect } from "util";
 import { PreparePackagesResult, getAffectedPackages } from "./get-affected-packages";
+import { parseVersionFromDirectoryName } from "./lib/definition-parser";
+import { AllPackages, NotNeededPackage, PackageId, formatTypingVersion, getDependencyFromFile } from "./packages";
+import { readFile } from "fs/promises";
 
 export type GitDiff =
   | {
@@ -51,17 +53,39 @@ export async function gitDiff(log: Logger, definitelyTypedPath: string, diffBase
   }
 }
 
+async function getAttwJson(definitelyTypedPath: string, diffBase: string) {
+  return {
+    base: JSON.parse(await execAndThrowErrors("git", ["show", `${diffBase}:attw.json`], definitelyTypedPath)) as { failingPackages: string[] },
+    head: JSON.parse(await readFile(joinPaths(definitelyTypedPath, "attw.json"), "utf8")) as { failingPackages: string[] },
+  };
+}
+
 /**
  * @returns packages with added or removed files, but not packages with only changed files;
  * {@link getAffectedPackages | those are found by calling pnpm }.
  */
-export function gitChanges(
+export async function gitChanges(
   diffs: GitDiff[],
-): { errors: string[] } | { deletions: PackageId[]; additions: PackageId[] } {
+  getAttwJson: () => Promise<{ base: { failingPackages: string[] }; head: { failingPackages: string[] } }>,
+): Promise<{ errors: string[]; } | { deletions: PackageId[]; additions: PackageId[]; attwChanges: PackageId[]; }> {
   const deletions: Map<string, PackageId> = new Map();
   const additions: Map<string, PackageId> = new Map();
+  let attwChanges: PackageId[] = [];
   const errors = [];
   for (const diff of diffs) {
+    if (diff.file === "attw.json") {
+      try {
+        const { base, head } = await getAttwJson();
+        attwChanges = Array.from(symmetricDifference(new Set(base.failingPackages), new Set(head.failingPackages))).map(p => {
+          const [typesDirectoryName, versionDirectory] = p.split("/", 2);
+          const version = parseVersionFromDirectoryName(versionDirectory) ?? "*";
+          return { typesDirectoryName, version };
+        })
+      } catch {
+        errors.push(`Error reading attw.json`);
+      }
+      continue;
+    }
     if (!/types[\\/]/.test(diff.file)) continue;
     if (diff.status === "M") continue;
     const dep = getDependencyFromFile(diff.file);
@@ -90,7 +114,7 @@ You should ` +
     }
   }
   if (errors.length) return { errors };
-  return { deletions: Array.from(deletions.values()), additions: Array.from(additions.values()) };
+  return { deletions: Array.from(deletions.values()), additions: Array.from(additions.values()), attwChanges };
 }
 export async function getAffectedPackagesFromDiff(
   allPackages: AllPackages,
@@ -99,7 +123,7 @@ export async function getAffectedPackagesFromDiff(
 ): Promise<string[] | PreparePackagesResult> {
   const errors = [];
   const diffs = await gitDiff(consoleLogger.info, definitelyTypedPath, diffBase);
-  const git = gitChanges(diffs);
+  const git = await gitChanges(diffs, () => getAttwJson(definitelyTypedPath, diffBase));
   if ("errors" in git) {
     return git.errors;
   }
@@ -120,6 +144,7 @@ export async function getAffectedPackagesFromDiff(
   }
   console.log(`Testing ${affected.packageNames.size} changed packages: ${inspect(affected.packageNames)}`);
   console.log(`Testing ${affected.dependents.size} dependent packages: ${inspect(affected.dependents)}`);
+  console.log(`Testing ${affected.attwChanges.size} packages from attw.json changes: ${inspect(affected.attwChanges)}`);
   return affected;
 }
 

packages/definitions-parser/test/get-affected-packages.test.ts

@@ -29,6 +29,7 @@ testo({
       allPackages,
       packageOutput,
       [],
+      [],
       [dependentOutput],
       "/dt",
     );
@@ -42,6 +43,7 @@ testo({
       allPackages,
       packageOutput,
       [],
+      [],
       [dependentOutput],
       "/dt",
     );
@@ -57,7 +59,7 @@ testo({
       `/dt/types/has-older-test-dependency
 /dt/types/known`,
     ];
-    const { packageNames } = await getAffectedPackagesWorker(allPackages, packageOutput, [], dependentOutput, "/dt");
+    const { packageNames } = await getAffectedPackagesWorker(allPackages, packageOutput, [], [], dependentOutput, "/dt");
     expect(packageNames).toEqual(new Set(["jquery"]));
   },
   async newPackage() {
@@ -67,6 +69,7 @@ testo({
       allPackages,
       packageOutput,
       ["mistake"],
+      [],
       [dependentOutput],
       "/dt",
     );
@@ -82,6 +85,7 @@ testo({
       allPackages,
       packageOutput,
       [],
+      [],
       [dependentOutput],
       "/dt",
     );

packages/definitions-parser/test/git.test.ts

@@ -59,19 +59,25 @@ const moveRdfJSDiffs: GitDiff[] = [
     file: "types/rdfjs__serializer-turtle/tsconfig.json",
   },
 ];
-function getDeletions(diffs: GitDiff[]): PackageId[] {
-  const changes = gitChanges(diffs);
+
+function createGetAttwJson(base: string[] = [], head: string[] = []) {
+  return async () => ({ base: { failingPackages: base }, head: { failingPackages: head } });
+}
+
+async function getDeletions(diffs: GitDiff[]): Promise<PackageId[]> {
+  const changes = await gitChanges(diffs, createGetAttwJson());
   expect(changes).not.toHaveProperty("error");
   const { deletions } = changes as { deletions: PackageId[]; additions: PackageId[] };
   return deletions;
 }
 
 testo({
   async ok() {
-    expect(await getNotNeededPackages(allPackages, getDeletions(deleteJestDiffs))).toEqual(jestNotNeeded);
+    expect(await getNotNeededPackages(allPackages, await getDeletions(deleteJestDiffs))).toEqual(jestNotNeeded);
   },
   async gitMovesConvertedToAddsAndDeletes() {
-    expect(gitChanges(moveRdfJSDiffs)).toEqual({
+    expect(await gitChanges(moveRdfJSDiffs, createGetAttwJson())).toEqual({
+      attwChanges: [],
       additions: [
         { typesDirectoryName: "rdf-ext", version: "*" },
         { typesDirectoryName: "rdfjs__formats", version: "*" },
@@ -88,39 +94,56 @@ testo({
     expect(
       await getNotNeededPackages(
         AllPackages.fromTestData({ jest: createTypingsVersionRaw("jest", {}, {}) }, jestNotNeeded),
-        getDeletions(deleteJestDiffs),
+        await getDeletions(deleteJestDiffs),
       ),
     ).toEqual({ errors: ["Please delete all files in jest when adding it to notNeededPackages.json."] });
   },
   async tooManyDeletes() {
     expect(
-      await getNotNeededPackages(allPackages, getDeletions([{ status: "D", file: "types/oops/oops.txt" }])),
+      await getNotNeededPackages(allPackages, await getDeletions([{ status: "D", file: "types/oops/oops.txt" }])),
     ).toEqual([]);
   },
   async deleteInOtherPackage() {
     expect(
       await getNotNeededPackages(
         allPackages,
-        getDeletions([...deleteJestDiffs, { status: "D", file: "types/most-recent/extra-tests.ts" }]),
+        await getDeletions([...deleteJestDiffs, { status: "D", file: "types/most-recent/extra-tests.ts" }]),
       ),
     ).toEqual(jestNotNeeded);
   },
   async extraneousFile() {
     expect(
       await getNotNeededPackages(
         allPackages,
-        getDeletions([...deleteJestDiffs, { status: "A", file: "types/oops/oooooooooooops.txt" }]),
+        await getDeletions([...deleteJestDiffs, { status: "A", file: "types/oops/oooooooooooops.txt" }]),
       ),
     ).toEqual(jestNotNeeded);
   },
   async scoped() {
     expect(
       await getNotNeededPackages(
         AllPackages.fromTestData(typesData, [new NotNeededPackage("ember__object", "@ember/object", "1.0.0")]),
-        getDeletions([{ status: "D", file: "types/ember__object/index.d.ts" }]),
+        await getDeletions([{ status: "D", file: "types/ember__object/index.d.ts" }]),
       ),
     ).toEqual([new NotNeededPackage("ember__object", "@ember/object", "1.0.0")]);
   },
+  async attwChanges() {
+    expect(await gitChanges([
+      { status: "M", file: "attw.json" },
+    ], createGetAttwJson(
+      ["lodash", "jquery/v1", "react"],
+      ["jquery/v2", "react", "new-package"]
+    ))).toEqual({
+      additions: [],
+      deletions: [],
+      attwChanges: [
+        { typesDirectoryName: "lodash", version: "*" },
+        { typesDirectoryName: "jquery", version: { major: 1, minor: undefined } },
+        { typesDirectoryName: "jquery", version: { major: 2, minor: undefined } },
+        { typesDirectoryName: "new-package", version: "*" },
+      ]
+    });
+  }
   // TODO: Test npm info (and with scoped names)
   // TODO: Test with dependents, etc etc
 });

packages/dtslint-runner/src/main.ts

@@ -42,15 +42,15 @@ export async function runDTSLint({
 
   const typesPath = joinPaths(definitelyTypedPath, "types");
 
-  const { packageNames, dependents } = onlyRunAffectedPackages
+  const { packageNames, dependents, attwChanges } = onlyRunAffectedPackages
     ? await prepareAffectedPackages(definitelyTypedPath, diffBase)
     : await prepareAllPackages(definitelyTypedPath, definitelyTypedAcquisition.kind === "clone");
 
   const allFailures: [string, string][] = [];
   const allWarnings: [string, string][] = [];
   const expectedFailures = getExpectedFailures(onlyRunAffectedPackages, dependents);
 
-  const allPackages = [...packageNames, ...dependents];
+  const allPackages = [...packageNames, ...attwChanges, ...dependents];
   const testedPackages = shard ? allPackages.filter((_, i) => i % shard.count === shard.id - 1) : allPackages;
 
   const dtslintArgs = [
@@ -64,7 +64,7 @@ export async function runDTSLint({
       path,
       onlyTestTsNext,
       expectOnly,
-      skipNpmChecks: skipNpmChecks || !packageNames.has(path),
+      npmChecks: attwChanges.has(path) ? "only" : expectOnly || skipNpmChecks ? false : true,
     })),
     commandLineArgs: dtslintArgs,
     workerFile: require.resolve("@definitelytyped/dtslint"),

packages/dtslint-runner/src/prepareAllPackages.ts

@@ -20,6 +20,7 @@ export async function prepareAllPackages(definitelyTypedPath: string, clone: boo
   return {
     packageNames: new Set((await allPackages.allTypings()).map(({ subDirectoryPath }) => subDirectoryPath)),
     dependents: new Set(),
+    attwChanges: new Set(),
   };
 }
 const npmRetryCount = 5;