test(unit-tests): make the harness runnable again

[chrip]

Make the unit-test harness runnable again

test/unit-tests/common/index.html could not run at all. Every path in it, and in
the two test files it loads, pointed into a web-apps — копия directory that is not
in this repository. RequireJS aborts the whole run on the first 404, so the suite has
been dead long enough for the rest of it to rot behind that.

Fixed

• Paths resolve inside this checkout. baseUrl was '../../apps/', which from
  test/unit-tests/common/ is test/apps/ — a directory that has never existed.
• mocha.setup() no longer passes ignoreLeaks. Removed in mocha 4, and
  setup() calls every key as a method, so an unknown one throws
  self[opt] is not a function before a single test registers.
• chai 5 is ESM-only ("type": "module", no UMD build), so RequireJS's classic
  script tag dies on export. The page imports it as a module and registers it under
  the id the test files already use, leaving define(['chai']) and require('chai')
  untouched.
• jquery / underscore / backbone load before the tests and are published to
  window. The components read them as globals without declaring them as
  dependencies, and underscore's UMD build registers as AMD without leaving a global
  behind, so RequireJS was free to evaluate a component first.
• .js-suffixed module ids resolve against the page rather than baseUrl — the
  ids inside the test files lost their extension, the ones in index.html kept theirs.
• The test setup module loads before the suites. It publishes assert/expect
  and stubs the ambient Common.* state, and it sat in the same flat require array
  as the suites that read them. RequireJS documents no ordering guarantee for
  independent siblings in one array, so the call is nested — the same pattern already
  used for the vendor globals one level up.

And the Button suite, which was failing on its own terms

• .andSelf() was removed in jQuery 3; it is .addBack() now.
• Common.UI.Scaling.currentRatio(), Common.Locale.isCurrentLanguageRtl() and
  Common.NotificationCenter are read at render time. The real modules pull in core
  and the whole application bootstrap, which is the opposite of a unit test, so
  common.js stubs the three.

Result

12 tests, no failures. Serve the repository over http and open
test/unit-tests/common/index.html — RequireJS cannot load modules from file://,
where Chrome gives every URL its own opaque origin.

No product code is touched.

No longer depends on #130

The SmartPicker suite registration moved to #130, which is where the module
(apps/common/main/lib/util/SmartPicker.js) and its test file come from. This branch
now stands on its own and the two can merge in either order; whichever lands second
resolves a one-line conflict in the require array.

Assisted-by: ClaudeCode:claude-opus-5

[moodyjmz]

TL;DR: Request changes — one real, previously-unreported bug (a module load-order race that can blank out the whole suite), plus a disclosed-but-technically-unenforced dependency on #130. Everything else in the PR checks out, verified by actually running the harness in a real browser, not just reading the diff.

Full review — verified by cloning, checking out the branch, and running the harness in headless Chrome

1. Real bug: module load-order race in test/unit-tests/common/index.html.

The test-setup module (../test/unit-tests/common, which sets window.assert/window.expect and stubs Common.UI.Scaling/Common.Locale/Common.NotificationCenter) is placed in the same flat require([...]) array as the test files that read those globals:

require([
    '../test/unit-tests/common',
    './main/lib/util/utils.js',
    './main/lib/component/Button.js',
    './main/lib/util/SmartPicker.js'
], runMocha);

RequireJS does not guarantee array-order execution — it resolves whichever module's fetch completes first. This currently "works" only because the setup file is small and fast on localhost. Proof: I wrote a small proxy that delays serving common.js by 2s and reran the suite — result was a hard failure, Module name "chai" has not been loaded yet for context: _, blank page, 0 tests registered. This reproduced identically on two independent runs (mine and a second cold pass).

Fix — nest the require so the setup module is guaranteed to finish first (verified: 26/26 pass even under the same 2s injected delay once nested):

require(['../test/unit-tests/common'], function () {
    require([
        './main/lib/util/utils.js',
        './main/lib/component/Button.js',
        './main/lib/util/SmartPicker.js'
    ], runMocha);
});

This is the same pattern the PR already uses correctly for jquery/underscore/backbone — just needs to be applied one level up as well.

2. Confirmed as described, but not technically enforced: the PR depends on #130.

Reproduced directly: this branch's diff, merged alone (before #130 lands), 404s on SmartPicker.js and aborts with zero tests run. With #130's two files (apps/common/main/lib/util/SmartPicker.js and its test) present, 26/26 pass. The PR body discloses this ("⚠️ Depends on #130 — merge this second"), which is good, but nothing enforces the order — not draft, no label, no branch protection — and no CI runs this suite at all (checked all workflows; no references to unit-tests or mocha). If #194 merges before #130, the harness silently goes back to fully broken, and nothing will flag it until someone opens the page by hand.

3. Everything else checks out — verified live, not just read:

• jQuery 3.7.1 is bundled (.andSelf() genuinely removed, .addBack() is correct) — confirmed via version check and a passing Button test.
• build/node_modules/chai/package.json is genuinely "type": "module" with no UMD build — the <script type="module"> import + manual define('chai', ...) registration is a legitimate workaround, not a hack.
• mocha.setup() dropping ignoreLeaks is correct for mocha 10 (option was removed in mocha 4; passing an unknown key throws before any test registers).
• The baseUrl fix ('../../apps/' → '../../../apps/') resolves correctly in practice — reran with the original broken value and got the same 404-and-abort failure the PR describes.
• No product code touched — all four changed files are under test/unit-tests/.
• The "26 tests" (PR body) vs. "12 tests" (commit message) figures aren't a discrepancy — reproduced both exactly: 12 without SmartPicker's test file, 26 with it. Two accurate snapshots at different points in the branch's history, not sloppy reporting.

(Review assisted by Claude Code — findings verified by actually running the test harness in headless Chrome, including a live repro of the race condition, not inferred from the diff or the PR description.)

[chrip]

@moodyjmz I fixed the race condition and removed the dependency to #130

[moodyjmz]

TL;DR: Both findings from the previous review are genuinely fixed — verified by re-running, not just reading the diff. One new, small blocker: the fix commit is missing DCO sign-off.

Re-verification

1. Race condition — fixed, confirmed. require() is now nested (setup module resolves before the test files load, matching the pattern already used for jquery/underscore/backbone). I reran the same delay-injection test as before — 2s artificial delay on common.js — and the suite now passes 12/12 with no abort. Previously this blanked the page (Module name "chai" has not been loaded yet, 0 tests).

2. #130 dependency — genuinely removed. The SmartPicker.js require entry is gone. Ran the harness standalone with zero #130 files present anywhere in the tree: 12/12 pass, no 404, no abort. This branch stands on its own now, as the commit message says.

3. New blocker: DCO. Commit 96e788e ("test: load the test setup before the suites, drop the SmartPicker entry") has no Signed-off-by: trailer — the original commit does. That's why the DCO check is now ACTION_REQUIRED and the PR is blocked. Needs git commit --amend -s (or an empty sign-off commit) + force-push.

Nothing else regressed — the jQuery/chai/mocha mechanics I verified live in the previous pass are untouched.

(Re-reviewed by re-cloning, checking out the updated branch, and re-running the harness — including repeating the delay-injection repro — rather than trusting the commit message.)

[moodyjmz]

Race condition and #130 dependency both verified fixed (re-ran the harness live, including the delay-injection repro). DCO now green. Approving.