Dev

.github/scripts/__tests__/apk-selection.test.js

@@ -6,7 +6,7 @@ const path = require('node:path');
 const os = require('node:os');
 const {
   extractVersionFromString,
-  scoreApk,
+  filenameToCandidate,
   findCachedApk,
   findPackageCandidate,
   bestRankedApkInDir,
@@ -28,60 +28,78 @@ describe('extractVersionFromString', () => {
   });
 });
 
-describe('scoreApk', () => {
-  // The weights live in apk-selection.js and were lifted directly from the
-  // original inline awk score() function. These tests guard the scoring
-  // contract that findPackageCandidate / bestRankedApkInDir rely on.
+describe('filenameToCandidate', () => {
+  // filenameToCandidate is the directory-scan→candidate bridge that
+  // replaced the old regex-based `scoreApk`. These tests pin the
+  // extraction contract that findPackageCandidate/bestRankedApkInDir
+  // rely on; the actual ranking is exercised by those higher-level
+  // tests further down.
 
-  test('arm64-v8a .apk with no negatives scores very high', () => {
-    const s = scoreApk('/dir/app_arm64-v8a.apk');
-    // 2000 (.apk) + 800 (arm64) = 2800
-    expect(s).toBe(2800);
+  test('extracts arm64-v8a from a filename carrying the tag', () => {
+    const c = filenameToCandidate('/dir/something_arm64-v8a.apk');
+    expect(c.architecture).toBe('arm64-v8a');
+    expect(c.format).toBe('apk');
+    expect(c.source).toBe('directory-scan');
+    expect(c.url.endsWith('something_arm64-v8a.apk')).toBe(true);
   });
 
-  test('arm64-v8a base.apk is the absolute best candidate', () => {
-    // The +500 "base.apk" bonus only applies when the file is exactly
-    // named "base.apk" (the awk uses `b == "base.apk"`); the
-    // arm64 match adds 800.
-    const s = scoreApk('/dir/base.apk');
-    // 2000 (.apk) + 800 (arm64 doesn't match — filename has no arm64) = 2000
-    // Actually "base.apk" doesn't match arm64, so just 2000 + 500 (base.apk) = 2500.
-    expect(s).toBe(2500);
+  test('extracts x86_64 from an underscored/dashed filename', () => {
+    expect(filenameToCandidate('/dir/app_x86_64.apk').architecture).toBe('x86_64');
+    expect(filenameToCandidate('/dir/app_x86-64.apk').architecture).toBe('x86_64');
   });
 
-  test('arm64-v8a base.apk scores higher than arm64-v8a app.apk (base.apk bonus)', () => {
-    // Same dir, base.apk named with arm64 in some other file vs arm64-v8a app.apk.
-    // We assert ordering instead of exact numbers to keep the test robust.
-    const baseArm = scoreApk('/dir/base.apk');                  // 2500 (no arm64 in name)
-    const appArm = scoreApk('/dir/app_arm64-v8a.apk');          // 2800
-    expect(appArm).toBeGreaterThan(baseArm); // arm64 wins alone
-    // But a base_arm64-v8a.apk beats both:
-    const baseAndArm = scoreApk('/tmp/base_arm64-v8a.apk');     // 2000 + 800 = 2800 (no base.apk bonus — basename != "base.apk")
-    expect(baseAndArm).toBeGreaterThan(baseArm);
+  test('arm64 detection picks arm64-v8a over the x86 sibling', () => {
+    expect(filenameToCandidate('/dir/libx86_split_config.arm64_v8a.apk').architecture).toBe('arm64-v8a');
   });
 
-  test('xapk splits are heavily demoted vs .apk', () => {
-    const apk = scoreApk('/dir/something_arm64-v8a.apk');
-    const xapk = scoreApk('/dir/something_arm64-v8a.xapk');
-    expect(apk).toBeGreaterThan(xapk);
+  test('armeabi-v7a (v7a shorthand) is recognized', () => {
+    // lib_v7a.so is a v7a-tagged .so file — the arch tag is in the
+    // filename, so extraction picks it up just like the legacy
+    // scoreApk would have applied a v7a penalty.
+    expect(filenameToCandidate('/dir/lib_v7a.so').architecture).toBe('armeabi-v7a');
+    expect(filenameToCandidate('/dir/app_armeabi-v7a.apk').architecture).toBe('armeabi-v7a');
   });
 
-  test('x86 architecture is penalized heavily', () => {
-    const arm = scoreApk('/dir/app_arm64-v8a.apk');  // 2000 + 800 = 2800
-    const x86 = scoreApk('/dir/app_x86_64.apk');     // 2000 - 600 = 1400
-    expect(arm).toBeGreaterThan(x86);
-    expect(x86).toBeLessThan(arm);
+  test('arm64 detection accepts arm64 (without the v8a suffix)', () => {
+    expect(filenameToCandidate('/dir/app_arm64.apk').architecture).toBe('arm64-v8a');
   });
 
-  test('split_config / config. artifacts are severely demoted', () => {
-    const config = scoreApk('/dir/split_config.arm64_v8a.apk');  // 2000 + 800 - 1400 = 1400
-    const normal = scoreApk('/dir/app_arm64-v8a.apk');           // 2800
-    expect(normal).toBeGreaterThan(config);
-    expect(config).toBe(1400);
+  test('arm64-v8a in arm64_suffixed filename is parsed', () => {
+    expect(filenameToCandidate('/dir/lib_arm64_v8a.so').architecture).toBe('arm64-v8a');
   });
 
-  test('case-insensitive', () => {
-    expect(scoreApk('/dir/APP_ARM64-V8A.APK')).toBe(scoreApk('/dir/app_arm64-v8a.apk'));
+  test('universal APKs are recognized', () => {
+    expect(filenameToCandidate('/dir/app_universal.apk').architecture).toBe('universal');
+  });
+
+  test('architecture defaults to "unknown" when no tag is present', () => {
+    const c = filenameToCandidate('/dir/base.apk');
+    expect(c.architecture).toBe('unknown');
+    expect(c.format).toBe('apk');
+  });
+
+  test('unsupported extension maps to "unknown" format', () => {
+    const c = filenameToCandidate('/dir/whatever.zip');
+    expect(c.format).toBe('unknown');
+  });
+
+  test('case-insensitive parsing', () => {
+    const lower = filenameToCandidate('/dir/APP_ARM64-V8A.APK');
+    const mixed = filenameToCandidate('/dir/app_arm64-v8a.apk');
+    expect(lower.architecture).toBe(mixed.architecture);
+    expect(lower.format).toBe(mixed.format);
+  });
+
+  test('sizeBytes reflects stat when the file exists', () => {
+    const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'fname-cand-'));
+    const apk = path.join(tmp, 'app.apk');
+    fs.writeFileSync(apk, 'x'.repeat(12345));
+    expect(filenameToCandidate(apk).sizeBytes).toBe(12345);
+  });
+
+  test('sizeBytes falls back to null on stat failure', () => {
+    const c = filenameToCandidate('/nonexistent/path/file.apk');
+    expect(c.sizeBytes).toBeNull();
   });
 });
 

.github/scripts/__tests__/unified-downloader-runcommand.test.js

@@ -0,0 +1,97 @@
+// .github/scripts/__tests__/unified-downloader-runcommand.test.js
+'use strict';
+
+// runCommand() is an internal helper that wraps child_process.execFile
+// with a timeout, returns a Promise, and emits a timeout error message
+// when the child exceeds the deadline.
+//
+// Two contracts are pinned here:
+//
+//   1. (Issue #2) Node's execFile enforces its own `timeout` option
+//      independently of callback usage — verified for Node >=24 via
+//      runtime test. The Promise must reject with the custom
+//      `Command timed out after ${timeout}ms: ${cmd}` message instead
+//      of the raw ETIMEDOUT/ERR_CHILD_PROCESS_STDIO_TIMEOUT surface.
+//      Earlier code used a manual `setTimeout` that doubled up on
+//      execFile's built-in timeout; the manual one is now gone.
+//
+//   2. (Issue #1) After resolve (early exit), the helper must not
+//      leave any pending timers referencing its closure alive. The
+//      bug being pinned: a manual `setTimeout` whose handle was
+//      never captured/stored, so a child that returned in 50ms still
+//      held the timer open for the full `timeout` duration
+//      (default 120s). With the manual timer removed AND with any
+//      surviving manual setTimeout cleaned up, there are zero
+//      pending timers after resolve — proven by counting
+//      setTimeout/clearTimeout calls across the run.
+//
+// `runCommand` is exported solely for this test; see the `// For
+// testing` comment at the export in unified-downloader.js.
+
+const { runCommand } = require('../unified-downloader');
+
+describe('runCommand', () => {
+  test('resolves with { stdout, stderr, code } on a successful child', async () => {
+    const result = await runCommand('printf', ['hello-runcommand']);
+    expect(result.code).toBe(0);
+    expect(result.stdout).toBe('hello-runcommand');
+  });
+
+  test('rejects with the timeout-specific message when the child exceeds options.timeout', async () => {
+    // Real timeout, not a fake-timer exercise — Node execFile's
+    // built-in timeout must actually kill the child. We use a tiny
+    // 150ms deadline so the test stays fast.
+    const deadline = 150;
+    const start = Date.now();
+    await expect(
+      runCommand('sleep', ['5'], { timeout: deadline }),
+    ).rejects.toThrow(/Command timed out after \d+ms: sleep/);
+    // Sanity: the rejection lands well before the child would have
+    // finished naturally (5s sleep) — proof the timeout was actually
+    // enforced, not just the rejection message tagged on later.
+    expect(Date.now() - start).toBeLessThan(2_000);
+  });
+
+  test('does not leak pending timers when the child exits early', async () => {
+    // Spy on global setTimeout/clearTimeout to count timer registrations
+    // across a successful runCommand call. With the manual timer
+    // removed (issue #2) AND any legacy manual setTimeout cleared on
+    // early exit (issue #1), the helper must register zero timers of
+    // its own that outlive the Promise. execFile's internal timeout
+    // scheduler is not part of Jest's observable setTimeout API.
+    const setTimeoutSpy = jest.spyOn(global, 'setTimeout');
+    const clearTimeoutSpy = jest.spyOn(global, 'clearTimeout');
+    const reset = () => {
+      setTimeoutSpy.mockClear();
+      clearTimeoutSpy.mockClear();
+    };
+    try {
+      reset();
+      const startCalls = setTimeoutSpy.mock.calls.length;
+      const startClears = clearTimeoutSpy.mock.calls.length;
+
+      // Fast child that finishes long before its 5s deadline.
+      await runCommand('echo', ['ok'], { timeout: 5_000 });
+
+      const newTimerCalls = setTimeoutSpy.mock.calls.length - startCalls;
+      const newClearCalls = clearTimeoutSpy.mock.calls.length - startClears;
+
+      // The legacy bug: runCommand registered an untracked setTimeout
+      // that was never cleared. With the fix, manual timers either
+      // never fire (issue #2: use execFile's built-in) or are
+      // clearTimeout'd on early exit (issue #1: every registration
+      // balanced by a clearTimeout). Both end up at zero net.
+      expect(newTimerCalls).toBe(0);
+      expect(newClearCalls).toBe(0);
+    } finally {
+      setTimeoutSpy.mockRestore();
+      clearTimeoutSpy.mockRestore();
+    }
+  });
+
+  test('rejects on non-zero exit code with stderr in the message', async () => {
+    await expect(
+      runCommand('sh', ['-c', 'echo oops 1>&2; exit 7']),
+    ).rejects.toThrow(/Command failed with code 7/);
+  });
+});