Fix mismatched app and test versions for indirect dependencies (#653)

elm-test was running tests with indirect dependencies always at latest
available version, regardless of whether the app under test had indirect
dependencies pinned to an earlier version.

This can lead to tests behaving different from real world apps.

The reason is elm-solve-deps always picks the first acceptable version
given to it in the versions array.

This commit fixes the issue by ensuring indirect dependencies' versions
are always sorted in this order:
- the pinned version first
- versions higher than the pinned version, in ascending order
- versions lower than the pinned version, in descending order

Co-authored-by: Simon Lydell <[email protected]>

Author: omnibs

lib/DependencyProvider.js

@@ -9,8 +9,15 @@ const collator = new Intl.Collator('en', { numeric: true }); // for sorting SemV
 
 // Initialization work done only once.
 wasm.init();
-const syncGetWorker /*: {| get: (string) => string, shutDown: () => void |} */ =
-  SyncGet.startWorker();
+// Lazily start the worker until needed.
+// This is important for the tests, which never exit otherwise.
+let syncGetWorker_ /*: void | typeof SyncGet.SyncGetWorker */ = undefined;
+function syncGetWorker() /*: typeof SyncGet.SyncGetWorker */ {
+  if (syncGetWorker_ === undefined) {
+    syncGetWorker_ = SyncGet.startWorker();
+  }
+  return syncGetWorker_;
+}
 
 // Cache of existing versions according to the package website.
 class OnlineVersionsCache {
@@ -55,7 +62,7 @@ class OnlineVersionsCache {
 
     // Complete cache with a remote call to the package server.
     const remoteUrl = remotePackagesUrl + '/since/' + (versionsCount - 1); // -1 to check if no package was deleted.
-    const newVersions = JSON.parse(syncGetWorker.get(remoteUrl));
+    const newVersions = JSON.parse(syncGetWorker().get(remoteUrl));
     if (newVersions.length === 0) {
       // Reload from scratch since it means at least one package was deleted from the registry.
       this.map = onlineVersionsFromScratch(cachePath, remotePackagesUrl);
@@ -104,10 +111,13 @@ class OnlineAvailableVersionLister {
     this.onlineCache = onlineCache;
   }
 
-  list(pkg /*: string */) /*: Array<string> */ {
+  list(
+    pkg /*: string */,
+    pinnedVersion /*: void | string */
+  ) /*: Array<string> */ {
     const memoVersions = this.memoCache.get(pkg);
     if (memoVersions !== undefined) {
-      return memoVersions;
+      return prioritizePinnedIndirectVersion(memoVersions, pinnedVersion);
     }
     const offlineVersions = readVersionsInElmHomeAndSort(pkg);
     const allVersionsSet = new Set(this.onlineCache.getVersions(pkg));
@@ -117,24 +127,27 @@ class OnlineAvailableVersionLister {
     }
     const allVersions = [...allVersionsSet].sort(flippedSemverCompare);
     this.memoCache.set(pkg, allVersions);
-    return allVersions;
+    return prioritizePinnedIndirectVersion(allVersions, pinnedVersion);
   }
 }
 
 class OfflineAvailableVersionLister {
   // Memoization cache to avoid doing the same work twice in list.
   cache /*: Map<string, Array<string>> */ = new Map();
 
-  list(pkg /*: string */) /*: Array<string> */ {
+  list(
+    pkg /*: string */,
+    pinnedVersion /*: void | string */
+  ) /*: Array<string> */ {
     const memoVersions = this.cache.get(pkg);
     if (memoVersions !== undefined) {
-      return memoVersions;
+      return prioritizePinnedIndirectVersion(memoVersions, pinnedVersion);
     }
 
     const offlineVersions = readVersionsInElmHomeAndSort(pkg);
 
     this.cache.set(pkg, offlineVersions);
-    return offlineVersions;
+    return prioritizePinnedIndirectVersion(offlineVersions, pinnedVersion);
   }
 }
 
@@ -162,13 +175,21 @@ class DependencyProvider {
     extra /*: { [string]: string } */
   ) /*: string */ {
     const lister = new OfflineAvailableVersionLister();
+    const dependencies = JSON.parse(elmJson).dependencies;
+    const indirectDeps =
+      dependencies === undefined ? undefined : dependencies.indirect;
+
     try {
       return wasm.solve_deps(
         elmJson,
         useTest,
         extra,
         fetchElmJsonOffline,
-        (pkg) => lister.list(pkg)
+        (pkg) =>
+          lister.list(
+            pkg,
+            indirectDeps === undefined ? undefined : indirectDeps[pkg]
+          )
       );
     } catch (errorMessage) {
       throw new Error(errorMessage);
@@ -182,13 +203,21 @@ class DependencyProvider {
     extra /*: { [string]: string } */
   ) /*: string */ {
     const lister = new OnlineAvailableVersionLister(this.cache);
+    const dependencies = JSON.parse(elmJson).dependencies;
+    const indirectDeps =
+      dependencies === undefined ? undefined : dependencies.indirect;
+
     try {
       return wasm.solve_deps(
         elmJson,
         useTest,
         extra,
         fetchElmJsonOnline,
-        (pkg) => lister.list(pkg)
+        (pkg) =>
+          lister.list(
+            pkg,
+            indirectDeps === undefined ? undefined : indirectDeps[pkg]
+          )
       );
     } catch (errorMessage) {
       throw new Error(errorMessage);
@@ -208,7 +237,7 @@ function fetchElmJsonOnline(
     // or because there was an error parsing `pkg` and `version`.
     // In such case, this will throw again with `cacheElmJsonPath()` so it's fine.
     const remoteUrl = remoteElmJsonUrl(pkg, version);
-    const elmJson = syncGetWorker.get(remoteUrl);
+    const elmJson = syncGetWorker().get(remoteUrl);
     const cachePath = cacheElmJsonPath(pkg, version);
     const parentDir = path.dirname(cachePath);
     fs.mkdirSync(parentDir, { recursive: true });
@@ -239,7 +268,7 @@ function onlineVersionsFromScratch(
   cachePath /*: string */,
   remotePackagesUrl /*: string */
 ) /*: Map<string, Array<string>> */ {
-  const onlineVersionsJson = syncGetWorker.get(remotePackagesUrl);
+  const onlineVersionsJson = syncGetWorker().get(remotePackagesUrl);
   fs.writeFileSync(cachePath, onlineVersionsJson);
   const onlineVersions = JSON.parse(onlineVersionsJson);
   try {
@@ -253,6 +282,42 @@ function onlineVersionsFromScratch(
 
 // Helper functions ##################################################
 
+/**
+ * Enforces respecting pinned indirect dependencies.
+ *
+ * When Elm apps have pinned indirect versions, e.g.:
+ *
+ * "indirect": {
+ *   "elm/virtual-dom": "1.0.3"
+ * }
+ *
+ * We must prioritize these versions for the wasm dependency solver.
+ *
+ * Otherwise the wasm solver will take liberties that will result in
+ * tests running with dependency versions distinct from those used by
+ * the real live application.
+ *
+ * Assumes versions is sorted descending (newest -> oldest).
+ */
+function prioritizePinnedIndirectVersion(
+  versions /*: Array<string> */,
+  pinnedVersion /*: void | string */
+) /*: Array<string> */ {
+  if (pinnedVersion === undefined || !versions.includes(pinnedVersion)) {
+    return versions;
+  }
+
+  // the pinned version and any newer version, in ascending order
+  const desirableVersions = versions
+    .filter((v) => v >= pinnedVersion)
+    .reverse();
+
+  // older versions, in descending order
+  const olderVersions = versions.filter((v) => v < pinnedVersion);
+
+  return desirableVersions.concat(olderVersions);
+}
+
 /* Compares two versions so that newer versions appear first when sorting with this function. */
 function flippedSemverCompare(a /*: string */, b /*: string */) /*: number */ {
   return collator.compare(b, a);
@@ -340,4 +405,7 @@ function splitPkgVersion(str /*: string */) /*: {
   return { pkg: parts[0], version: parts[1] };
 }
 
-module.exports = DependencyProvider;
+module.exports = {
+  DependencyProvider,
+  prioritizePinnedIndirectVersion,
+};

lib/Generate.js

@@ -3,7 +3,7 @@
 const { supportsColor } = require('chalk');
 const fs = require('fs');
 const path = require('path');
-const DependencyProvider = require('./DependencyProvider.js');
+const { DependencyProvider } = require('./DependencyProvider.js');
 const ElmJson = require('./ElmJson');
 const Project = require('./Project');
 const Report = require('./Report');

lib/RunTests.js

@@ -6,7 +6,7 @@ const path = require('path');
 const readline = require('readline');
 const packageInfo = require('../package.json');
 const Compile = require('./Compile');
-const DependencyProvider = require('./DependencyProvider.js');
+const { DependencyProvider } = require('./DependencyProvider.js');
 const ElmJson = require('./ElmJson');
 const FindTests = require('./FindTests');
 const Generate = require('./Generate');

lib/Solve.js

@@ -5,7 +5,7 @@ const fs = require('fs');
 const path = require('path');
 const ElmJson = require('./ElmJson');
 const Project = require('./Project');
-const DependencyProvider = require('./DependencyProvider.js');
+const { DependencyProvider } = require('./DependencyProvider.js');
 
 // These value are used _only_ in flow types. 'use' them with the javascript
 // void operator to keep eslint happy.

lib/SyncGet.js

@@ -8,12 +8,19 @@ const {
   // $FlowFixMe[cannot-resolve-module]: Flow doesn’t seem to know about the `worker_threads` module yet.
 } = require('worker_threads');
 
-// Start a worker thread and return a `syncGetWorker`
-// capable of making sync requests until shut down.
-function startWorker() /*: {
+// Poor man’s type alias. We can’t use /*:: type SyncGetWorker = ... */ because of:
+// https://github.com/prettier/prettier/issues/2597
+const SyncGetWorker /*: {
   get: (string) => string,
   shutDown: () => void,
-} */ {
+} */ = {
+  get: (string) => string,
+  shutDown: () => {},
+};
+
+// Start a worker thread and return a `syncGetWorker`
+// capable of making sync requests until shut down.
+function startWorker() /*: typeof SyncGetWorker */ {
   const { port1: localPort, port2: workerPort } = new MessageChannel();
   const sharedLock = new SharedArrayBuffer(4);
   // $FlowFixMe[incompatible-call]: Flow is wrong and says `sharedLock` is not an accepted parameter here.
@@ -41,5 +48,6 @@ function startWorker() /*: {
 }
 
 module.exports = {
+  SyncGetWorker,
   startWorker,
 };

lib/elm-test.js

@@ -7,7 +7,7 @@ const path = require('path');
 const which = require('which');
 const packageInfo = require('../package.json');
 const Compile = require('./Compile');
-const DependencyProvider = require('./DependencyProvider.js');
+const { DependencyProvider } = require('./DependencyProvider.js');
 const ElmJson = require('./ElmJson');
 const FindTests = require('./FindTests');
 const Generate = require('./Generate');

tests/DependencyProvider.js

@@ -0,0 +1,50 @@
+'use strict';
+
+const assert = require('assert');
+const {
+  prioritizePinnedIndirectVersion,
+} = require('../lib/DependencyProvider');
+
+describe('DependencyProvider', () => {
+  describe('prioritizePinnedIndirectVersion', () => {
+    const versions = ['1.0.5', '1.0.4', '1.0.3', '1.0.2', '1.0.1', '1.0.0'];
+
+    const testPinning = (pinnedVersion, expectedResult) => {
+      assert.deepStrictEqual(
+        prioritizePinnedIndirectVersion(versions, pinnedVersion),
+        expectedResult
+      );
+    };
+
+    it('retains order when no pinned indirect dependency', () => {
+      testPinning(undefined, versions);
+    });
+
+    it("retains order when pinned version doesn't exist", () => {
+      testPinning('1.0.6', versions);
+    });
+
+    it('retains order if already at latest', () => {
+      testPinning('1.0.5', versions);
+    });
+
+    it("prioritizes a version in the middle, if we're pinned to it", () => {
+      const expected = [
+        // first, try the pinned version
+        '1.0.3',
+        // then, try upgrading
+        '1.0.4',
+        '1.0.5',
+        // then, try downgrading
+        '1.0.2',
+        '1.0.1',
+        '1.0.0',
+      ];
+      testPinning('1.0.3', expected);
+    });
+
+    it("prioritizes first version, if we're pinned to it", () => {
+      testPinning('1.0.0', [...versions].sort());
+    });
+  });
+});