Skip to content

Commit 8c7cea0

Browse files
test(coverage): expand unit tests for core helpers and handlers
Incrementally improve test coverage by adding focused tests for: - Alert handling (SHOW_ALERT/HIDE_ALERT with timer conversion) - Notification wrappers (socket/value/simple/delayed routing) - Action and data handler factories (getActionHandlers, getDataHandlers) - Config utilities (getConfigPath, getModuleDir, getConfig merging) - Helper utilities (translate, sendResponse, checkForExecError, getIpAddresses) - Changelog retrieval (answerGetChangelog error handling) - Socket notifications (CURRENT_STATUS, REQUEST_DEFAULT_SETTINGS, REMOTE_ACTION) Coverage improvements: - Statements: 39% → 49% (+10%) - Functions: 29% → 57% (+28%) - Branches: 71% → 83% (+12%) - Total tests: 81 → 152 (+71 tests, +88%) All new tests follow incremental testing strategy: minimal mocking, high-value assertions, focused on catching real regressions. Update coverage thresholds to reflect achieved stability (45%/50%/75%) and document why we stop here (diminishing returns, hardware/system commands require E2E testing).
1 parent aa7a3c8 commit 8c7cea0

9 files changed

Lines changed: 1005 additions & 29 deletions

package.json

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -90,9 +90,9 @@
9090
"**/*.md"
9191
],
9292
"check-coverage": true,
93-
"branches": 60,
94-
"lines": 30,
95-
"functions": 20,
96-
"statements": 30
93+
"branches": 75,
94+
"lines": 45,
95+
"functions": 50,
96+
"statements": 45
9797
}
9898
}

tests/README.md

Lines changed: 41 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -53,13 +53,22 @@ All tests run in CI/CD without MagicMirror runtime or browser dependencies.
5353
| `executeQuery.core.test.js` | Module visibility, notifications, system actions (SHOW/HIDE/REFRESH/RESTART) |
5454
| `executeQuery.error.test.js` | Error handling for malformed JSON, missing params |
5555
| `api.helpers.test.js` | JSON payload parsing, delay parameter handling |
56+
| `executeQuery.alerts.test.js` | Alert handling (SHOW_ALERT/HIDE_ALERT parameter handling) |
57+
| `executeQuery.notifications.test.js` | Notification wrapper functions and routing logic |
58+
| `executeQuery.handlers.test.js` | Action handlers, delayed queries, callback scheduling |
59+
| `helper.config.test.js` | Config path resolution, module directory handling |
60+
| `helper.utils.test.js` | Response handling, translation, error checking, network utilities |
61+
| `answerGetChangelog.test.js` | Changelog file retrieval and error handling |
62+
| `socketNotification.test.js` | Socket notification routing (CURRENT_STATUS, REQUEST_DEFAULT_SETTINGS, REMOTE_ACTION) |
63+
| `helper.getConfig.test.js` | Config merging with defaults from moduleData |
5664
| `utils.test.js`, `configUtils.test.js` | String-format helpers and `cleanConfig` regressions |
5765

5866
### DOM tests (`tests/dom/`)
5967

60-
| Suite | Purpose |
61-
| ---------------------- | ---------------------------------------------------------- |
62-
| `remote.smoke.test.js` | Frontend logic tests using happy-dom (no browser required) |
68+
| Suite | Purpose |
69+
| ---------------------------- | -------------------------------------------------------------------- |
70+
| `remote.smoke.test.js` | Frontend logic tests using happy-dom (no browser required) |
71+
| `MMM-Remote-Control.test.js` | Module initialization and lockStrings handling with edge case checks |
6372

6473
### HTTP-layer tests (`tests/integration/`)
6574

@@ -73,16 +82,16 @@ Together these suites focus on isolated logic (unit tests) and HTTP contract ver
7382

7483
As of December 2024, actual coverage stands at:
7584

76-
| Metric | Value | Threshold | Notes |
77-
| ---------------- | ----- | --------- | --------------------------------------- |
78-
| Statements | ~43% | 30% | Improved from ~26% with Priority 1+2 |
79-
| Branches | ~79% | 60% | Strong branch coverage |
80-
| Functions | ~46% | 20% | Improved from ~17% with edge case tests |
81-
| Lines | ~43% | 30% | Mirrors statement coverage |
82-
| `node_helper.js` | ~15% | - | Improved with state/value action tests |
83-
| `API/api.js` | ~62% | - | Edge cases covered |
85+
| Metric | Value | Threshold | Notes |
86+
| ---------------- | ----- | --------- | ------------------------------------------ |
87+
| Statements | ~49% | 30% | Reached plateau with core logic covered |
88+
| Branches | ~83% | 60% | Strong branch coverage |
89+
| Functions | ~57% | 20% | Major handlers and utilities tested |
90+
| Lines | ~49% | 30% | Mirrors statement coverage |
91+
| `node_helper.js` | ~28% | - | Core actions and notification flow covered |
92+
| `API/api.js` | ~62% | - | Edge cases and routing verified |
8493

85-
Thresholds now provide meaningful regression protection while remaining achievable.
94+
Thresholds now provide meaningful regression protection while remaining achievable. Further gains would require testing system commands and hardware integration better suited for E2E or manual testing.
8695

8796
## What we deliberately skip (and why)
8897

@@ -160,24 +169,31 @@ This approach catches route wiring bugs, middleware issues, and response format
160169
- Hardware-dependent command testing (shutdown, reboot, monitor control)
161170
- Git/network-dependent install/update flows
162171

163-
## Coverage improvement strategy
172+
## Coverage status and philosophy
164173

165-
To reach 50%+ coverage efficiently, follow an incremental approach focusing on **high-impact, low-mock areas**:
174+
As of December 2025, coverage has reached a healthy plateau through **incremental, high-value testing**:
166175

167-
1. Pick **one** untested function from `node_helper.js` or `API/api.js`
168-
2. Write test with **minimal setup** (reuse existing helper factories)
169-
3. Verify it catches a real regression (e.g., bounds checking, null handling)
170-
4. Check coverage gain and repeat until diminishing returns
176+
- **~49% statements** – Core logic paths covered
177+
- **~57% functions** – Major handlers tested
178+
- **~83% branches** – Strong conditional coverage
179+
- **152 tests** – Mix of unit, integration, and DOM tests
171180

172-
**Target: 50% statements** is realistic and valuable. Beyond that, focus shifts to integration/manual testing.
181+
### Why we stop here
173182

174-
### What NOT to chase:
183+
Further coverage gains would hit **diminishing returns**:
175184

176-
- **Don't test for coverage percentage alone** – Every test should catch real bugs or prevent regressions
177-
- **Avoid mocking complexity** – If a test needs >5 mocks, it's testing the wrong thing
178-
- **Skip brittle integration points** – File system, network, hardware, DOM manipulation
179-
- **Don't duplicate HTTP-layer tests** – Already covered at the right abstraction level
180-
- **Helper utilities** – Only add edge cases to `lib/configUtils.js` and `lib/utils.js` if behavior changes
185+
- Remaining untested code is primarily **system commands** (`shutdown`, `reboot`, monitor control) requiring hardware/privileges
186+
- **Complex integration points** (PM2, Electron, git operations) need E2E tests, not unit tests
187+
- **Express routing details** already verified at HTTP layer
188+
- Mock complexity would exceed test value
189+
190+
### Testing principles (when adding new tests)
191+
192+
- **Test behavior, not coverage** – Every test should catch real bugs or prevent regressions
193+
- **Minimal mocking** – If a test needs >5 mocks, reconsider the abstraction
194+
- **Skip brittle integration points** – File system, network, hardware, DOM manipulation better suited for E2E
195+
- **Avoid duplication** – Don't re-test what HTTP-layer tests already cover
196+
- **Edge cases only** – For utilities (`lib/configUtils.js`, `lib/utils.js`), add tests when behavior changes
181197

182198
---
183199

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
const assert = require("node:assert/strict");
2+
const {test, describe, before, after} = require("node:test");
3+
const path = require("node:path");
4+
const fs = require("node:fs");
5+
6+
// Add tests/shims to module resolution
7+
const ModuleLib = require("node:module");
8+
const shimDir = path.resolve(__dirname, "../shims");
9+
process.env.NODE_PATH = shimDir + (process.env.NODE_PATH ? path.delimiter + process.env.NODE_PATH : "");
10+
if (typeof ModuleLib._initPaths === "function") ModuleLib._initPaths();
11+
12+
const helperFactory = require("../../node_helper.js");
13+
14+
function freshHelper () {
15+
const h = Object.create(helperFactory);
16+
h.__responses = [];
17+
h.sendResponse = (res, error, data) => {
18+
h.__responses.push({err: error, data});
19+
if (res && res.json) {
20+
res.json(data || {success: !error});
21+
}
22+
};
23+
h.getModuleDir = () => "test_modules";
24+
return h;
25+
}
26+
27+
describe("answerGetChangelog", () => {
28+
const testModulesDir = path.join(__dirname, "../..", "test_modules");
29+
const testModuleDir = path.join(testModulesDir, "test-module");
30+
const changelogPath = path.join(testModuleDir, "CHANGELOG.md");
31+
32+
before(() => {
33+
// Create test module directory and CHANGELOG
34+
fs.mkdirSync(testModuleDir, {recursive: true});
35+
fs.writeFileSync(changelogPath, "# Changelog\n\n## v1.0.0\n- Initial release\n");
36+
});
37+
38+
after(() => {
39+
// Clean up test files
40+
try {
41+
fs.rmSync(testModulesDir, {recursive: true, force: true});
42+
} catch {
43+
// Ignore cleanup errors
44+
}
45+
});
46+
47+
test("returns changelog content when file exists", () => {
48+
const helper = freshHelper();
49+
helper.answerGetChangelog = helperFactory.answerGetChangelog.bind(helper);
50+
const mockRes = {
51+
json: (data) => {
52+
mockRes.jsonData = data;
53+
}
54+
};
55+
56+
helper.answerGetChangelog({module: "test-module"}, mockRes);
57+
58+
assert.equal(helper.__responses.length, 1);
59+
assert.equal(helper.__responses[0].err, undefined);
60+
assert.equal(helper.__responses[0].data.action, "GET_CHANGELOG");
61+
assert.equal(helper.__responses[0].data.module, "test-module");
62+
assert.ok(helper.__responses[0].data.changelog.includes("# Changelog"));
63+
assert.ok(helper.__responses[0].data.changelog.includes("Initial release"));
64+
});
65+
66+
test("returns error when changelog not found", () => {
67+
const helper = freshHelper();
68+
helper.answerGetChangelog = helperFactory.answerGetChangelog.bind(helper);
69+
const mockRes = {
70+
json: (data) => {
71+
mockRes.jsonData = data;
72+
}
73+
};
74+
75+
helper.answerGetChangelog({module: "nonexistent-module"}, mockRes);
76+
77+
assert.equal(helper.__responses.length, 1);
78+
assert.ok(helper.__responses[0].err instanceof Error);
79+
assert.equal(helper.__responses[0].err.message, "Changelog not found");
80+
assert.equal(helper.__responses[0].data.action, "GET_CHANGELOG");
81+
});
82+
83+
test("includes query in error response", () => {
84+
const helper = freshHelper();
85+
helper.answerGetChangelog = helperFactory.answerGetChangelog.bind(helper);
86+
const mockRes = {};
87+
const query = {module: "missing-module", other: "data"};
88+
89+
helper.answerGetChangelog(query, mockRes);
90+
91+
assert.equal(helper.__responses[0].data.query, query);
92+
});
93+
});
Lines changed: 176 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,176 @@
1+
const assert = require("node:assert/strict");
2+
const {test, describe} = require("node:test");
3+
4+
// Add tests/shims to module resolution
5+
const path = require("node:path");
6+
const ModuleLib = require("node:module");
7+
const shimDir = path.resolve(__dirname, "../shims");
8+
process.env.NODE_PATH = shimDir + (process.env.NODE_PATH ? path.delimiter + process.env.NODE_PATH : "");
9+
if (typeof ModuleLib._initPaths === "function") ModuleLib._initPaths();
10+
11+
const helperFactory = require("../../node_helper.js");
12+
13+
function freshHelper () {
14+
const h = Object.create(helperFactory);
15+
h.__socketNotifications = [];
16+
h.__responses = [];
17+
h.sendSocketNotification = (action, payload) => {
18+
h.__socketNotifications.push({action, payload});
19+
};
20+
h.sendResponse = (_res, error, data) => { h.__responses.push({err: error, data}); };
21+
h.waiting = [];
22+
return h;
23+
}
24+
25+
describe("callAfterUpdate", () => {
26+
test("schedules callback and sends UPDATE notification", () => {
27+
const helper = freshHelper();
28+
helper.callAfterUpdate = helperFactory.callAfterUpdate.bind(helper);
29+
let callbackExecuted = false;
30+
31+
helper.callAfterUpdate(() => {
32+
callbackExecuted = true;
33+
}, 50);
34+
35+
// Should add to waiting array
36+
assert.equal(helper.waiting.length, 1);
37+
assert.equal(helper.waiting[0].finished, false);
38+
39+
// Should send UPDATE notification
40+
assert.equal(helper.__socketNotifications.length, 1);
41+
assert.equal(helper.__socketNotifications[0].action, "UPDATE");
42+
43+
// Wait for timeout and trigger callback
44+
return new Promise((resolve) => {
45+
setTimeout(() => {
46+
helper.waiting[0].run();
47+
assert.ok(callbackExecuted, "Callback should be executed");
48+
resolve();
49+
}, 60);
50+
});
51+
});
52+
53+
test("waitObject.run() marks as finished and executes callback once", () => {
54+
const helper = freshHelper();
55+
helper.callAfterUpdate = helperFactory.callAfterUpdate.bind(helper);
56+
let callCount = 0;
57+
58+
helper.callAfterUpdate(() => {
59+
callCount++;
60+
});
61+
62+
const waitObject = helper.waiting[0];
63+
waitObject.run();
64+
assert.equal(waitObject.finished, true);
65+
assert.equal(callCount, 1);
66+
67+
// Second run should not execute callback
68+
waitObject.run();
69+
assert.equal(callCount, 1, "Callback should only execute once");
70+
});
71+
72+
test("uses default timeout of 3000ms when not specified", () => {
73+
const helper = freshHelper();
74+
helper.callAfterUpdate = helperFactory.callAfterUpdate.bind(helper);
75+
76+
helper.callAfterUpdate(() => {});
77+
78+
// Just verify it doesn't throw and creates wait object
79+
assert.equal(helper.waiting.length, 1);
80+
assert.ok(helper.waiting[0].callback);
81+
});
82+
});
83+
84+
describe("getActionHandlers", () => {
85+
test("returns object with all action handlers", () => {
86+
const helper = freshHelper();
87+
helper.thisConfig = {};
88+
helper.getActionHandlers = helperFactory.getActionHandlers.bind(helper);
89+
90+
const handlers = helper.getActionHandlers();
91+
92+
// Verify some key handlers exist
93+
assert.ok(typeof handlers.SHOW === "function");
94+
assert.ok(typeof handlers.HIDE === "function");
95+
assert.ok(typeof handlers.TOGGLE === "function");
96+
assert.ok(typeof handlers.BRIGHTNESS === "function");
97+
assert.ok(typeof handlers.REFRESH === "function");
98+
assert.ok(typeof handlers.SHOW_ALERT === "function");
99+
assert.ok(typeof handlers.NOTIFICATION === "function");
100+
assert.ok(typeof handlers.DELAYED === "function");
101+
});
102+
103+
test("handler functions are bound with correct context", () => {
104+
const helper = freshHelper();
105+
helper.thisConfig = {};
106+
helper.getActionHandlers = helperFactory.getActionHandlers.bind(helper);
107+
helper.handleSimpleNotification = helperFactory.handleSimpleNotification.bind(helper);
108+
109+
const handlers = helper.getActionHandlers();
110+
111+
// Call REFRESH handler
112+
handlers.REFRESH({action: "REFRESH"});
113+
114+
// Should send notification via helper's method
115+
assert.equal(helper.__socketNotifications.length, 1);
116+
assert.equal(helper.__socketNotifications[0].action, "REFRESH");
117+
});
118+
});
119+
120+
describe("getDataHandlers", () => {
121+
test("returns object with all data handlers", () => {
122+
const helper = freshHelper();
123+
helper.translation = {};
124+
helper.userPresence = true;
125+
helper.getConfig = () => ({});
126+
helper.getDataHandlers = helperFactory.getDataHandlers.bind(helper);
127+
128+
const handlers = helper.getDataHandlers();
129+
130+
// Verify key handlers exist
131+
assert.ok(typeof handlers.translations === "function");
132+
assert.ok(typeof handlers.config === "function");
133+
assert.ok(typeof handlers.brightness === "function");
134+
assert.ok(typeof handlers.temp === "function");
135+
assert.ok(typeof handlers.userPresence === "function");
136+
assert.ok(typeof handlers.classes === "function");
137+
});
138+
139+
test("translations handler returns translation data", () => {
140+
const helper = freshHelper();
141+
helper.translation = {HELLO: "Hello", WORLD: "World"};
142+
helper.sendResponse = helperFactory.sendResponse.bind(helper);
143+
helper.getDataHandlers = helperFactory.getDataHandlers.bind(helper);
144+
const mockRes = {
145+
status: () => mockRes,
146+
json: (data) => {
147+
mockRes.jsonData = data;
148+
}
149+
};
150+
151+
const handlers = helper.getDataHandlers();
152+
handlers.translations({data: "translations"}, mockRes);
153+
154+
assert.equal(mockRes.jsonData.success, true);
155+
assert.deepEqual(mockRes.jsonData.data, {HELLO: "Hello", WORLD: "World"});
156+
});
157+
158+
test("userPresence handler returns presence status", () => {
159+
const helper = freshHelper();
160+
helper.userPresence = false;
161+
helper.sendResponse = helperFactory.sendResponse.bind(helper);
162+
helper.getDataHandlers = helperFactory.getDataHandlers.bind(helper);
163+
const mockRes = {
164+
status: () => mockRes,
165+
json: (data) => {
166+
mockRes.jsonData = data;
167+
}
168+
};
169+
170+
const handlers = helper.getDataHandlers();
171+
handlers.userPresence({data: "userPresence"}, mockRes);
172+
173+
assert.equal(mockRes.jsonData.success, true);
174+
assert.equal(mockRes.jsonData.result, false);
175+
});
176+
});

0 commit comments

Comments
 (0)