gavin/MagicMirror
1
0
Fork 0
mirror of https://github.com/MagicMirrorOrg/MagicMirror.git synced 2026-03-12 17:51:41 +08:00
Code Issues Packages Projects Releases Wiki Activity

refactor(clientonly): modernize code structure and add comprehensive tests (#4022)

Browse Source 
This PR improves `clientonly` start option with better code structure,
validation, and comprehensive test coverage.

### Changes

**Refactoring:**
- Improved parameter handling with explicit function parameter passing
instead of closure
- Added port validation (1-65535) with proper NaN handling
- Removed unnecessary IIFE wrapper (Node.js modules are already scoped)
- Extracted `getCommandLineParameter` as a reusable top-level function
- Enhanced error reporting with better error messages
- Added connection logging for debugging

**Testing:**
- Added comprehensive e2e tests for parameter validation
- Test coverage for missing/incomplete parameters
- Test coverage for local address rejection (localhost, 127.0.0.1, ::1,
::ffff:127.0.0.1)
- Test coverage for port validation (invalid ranges, non-numeric values)
- Test coverage for TLS flag parsing
- Integration test with running server

### Testing

All tests pass:
```bash
npm test -- tests/e2e/clientonly_spec.js
# ✓ 18 tests passed
This commit is contained in:
Kristjan ESPERANTO
2026-01-28 20:48:47 +01:00
committed by GitHub
parent 6324ec2116
commit 2b55b8e0f4
2 changed files with 331 additions and 121 deletions
Download Patch File Download Diff File Expand all files Collapse all files

273
clientonly/index.js
View File

@@ -1,136 +1,167 @@
"use strict";
// Use separate scope to prevent global scope pollution
(function () {
const http = require("node:http");
const https = require("node:https");
/**
* Get command line parameters
* Assumes that a cmdline parameter is defined with `--key [value]`
*
* example: `node clientonly --address localhost --port 8080 --use-tls`
* @param {string} key key to look for at the command line
* @param {string} defaultValue value if no key is given at the command line
* @returns {string} the value of the parameter
*/
function getCommandLineParameter (key, defaultValue = undefined) {
const index = process.argv.indexOf(`--${key}`);
const value = index > -1 ? process.argv[index + 1] : undefined;
return value !== undefined ? String(value) : defaultValue;
}
/**
* Helper function to get server address/hostname from either the commandline or env
* @returns {object} config object containing address, port, and tls properties
*/
function getServerParameters () {
const config = {};
/**
* Helper function to get server address/hostname from either the commandline or env
*/
function getServerAddress () {
// Prefer command line arguments over environment variables
config.address = getCommandLineParameter("address", process.env.ADDRESS);
const portValue = getCommandLineParameter("port", process.env.PORT);
config.port = portValue ? parseInt(portValue, 10) : undefined;
/**
* Get command line parameters
* Assumes that a cmdline parameter is defined with `--key [value]`
* @param {string} key key to look for at the command line
* @param {string} defaultValue value if no key is given at the command line
* @returns {string} the value of the parameter
*/
function getCommandLineParameter (key, defaultValue = undefined) {
const index = process.argv.indexOf(`--${key}`);
const value = index > -1 ? process.argv[index + 1] : undefined;
return value !== undefined ? String(value) : defaultValue;
}
// determine if "--use-tls"-flag was provided
config.tls = process.argv.includes("--use-tls");
// Prefer command line arguments over environment variables
["address", "port"].forEach((key) => {
config[key] = getCommandLineParameter(key, process.env[key.toUpperCase()]);
});
return config;
}
// determine if "--use-tls"-flag was provided
config.tls = process.argv.indexOf("--use-tls") > 0;
}
/**
* Gets the config from the specified server url
* @param {string} url location where the server is running.
* @returns {Promise} the config
*/
function getServerConfig (url) {
// Return new pending promise
return new Promise((resolve, reject) => {
// Select http or https module, depending on requested url
const lib = url.startsWith("https") ? https : http;
const request = lib.get(url, (response) => {
let configData = "";
/**
* Gets the config from the specified server url
* @param {string} url location where the server is running.
* @returns {Promise} the config
*/
function getServerConfig (url) {
// Return new pending promise
return new Promise((resolve, reject) => {
// Select http or https module, depending on requested url
const lib = url.startsWith("https") ? require("node:https") : require("node:http");
const request = lib.get(url, (response) => {
let configData = "";
// Gather incoming data
response.on("data", function (chunk) {
configData += chunk;
});
// Resolve promise at the end of the HTTP/HTTPS stream
response.on("end", function () {
// Gather incoming data
response.on("data", function (chunk) {
configData += chunk;
});
// Resolve promise at the end of the HTTP/HTTPS stream
response.on("end", function () {
try {
resolve(JSON.parse(configData));
});
});
request.on("error", function (error) {
reject(new Error(`Unable to read config from server (${url} (${error.message}`));
} catch (parseError) {
reject(new Error(`Failed to parse server response as JSON: ${parseError.message}`));
}
});
});
}
/**
* Print a message to the console in case of errors
* @param {string} message error message to print
* @param {number} code error code for the exit call
*/
function fail (message, code = 1) {
if (message !== undefined && typeof message === "string") {
console.log(message);
} else {
console.log("Usage: 'node clientonly --address 192.168.1.10 --port 8080 [--use-tls]'");
}
process.exit(code);
}
request.on("error", function (error) {
reject(new Error(`Unable to read config from server (${url}) (${error.message})`));
});
});
}
getServerAddress();
(config.address && config.port) || fail();
const prefix = config.tls ? "https://" : "http://";
// Only start the client if a non-local server was provided
if (["localhost", "127.0.0.1", "::1", "::ffff:127.0.0.1", undefined].indexOf(config.address) === -1) {
getServerConfig(`${prefix}${config.address}:${config.port}/config/`)
.then(function (configReturn) {
// check environment for DISPLAY or WAYLAND_DISPLAY
const elecParams = ["js/electron.js"];
if (process.env.WAYLAND_DISPLAY) {
console.log(`Client: Using WAYLAND_DISPLAY=${process.env.WAYLAND_DISPLAY}`);
elecParams.push("--enable-features=UseOzonePlatform");
elecParams.push("--ozone-platform=wayland");
} else if (process.env.DISPLAY) {
console.log(`Client: Using DISPLAY=${process.env.DISPLAY}`);
} else {
fail("Error: Requires environment variable WAYLAND_DISPLAY or DISPLAY, none is provided.");
}
// Pass along the server config via an environment variable
const env = Object.create(process.env);
env.clientonly = true; // set to pass to electron.js
const options = { env: env };
configReturn.address = config.address;
configReturn.port = config.port;
configReturn.tls = config.tls;
env.config = JSON.stringify(configReturn);
// Spawn electron application
const electron = require("electron");
const child = require("node:child_process").spawn(electron, elecParams, options);
// Pipe all child process output to current stdout
child.stdout.on("data", function (buf) {
process.stdout.write(`Client: ${buf}`);
});
// Pipe all child process errors to current stderr
child.stderr.on("data", function (buf) {
process.stderr.write(`Client: ${buf}`);
});
child.on("error", function (err) {
process.stdout.write(`Client: ${err}`);
});
child.on("close", (code) => {
if (code !== 0) {
console.log(`There something wrong. The clientonly is not running code ${code}`);
}
});
})
.catch(function (reason) {
fail(`Unable to connect to server: (${reason})`);
});
/**
* Print a message to the console in case of errors
* @param {string} message error message to print
* @param {number} code error code for the exit call
*/
function fail (message, code = 1) {
if (message !== undefined && typeof message === "string") {
console.error(message);
} else {
fail();
console.error("Usage: 'node clientonly --address 192.168.1.10 --port 8080 [--use-tls]'");
}
}());
process.exit(code);
}
/**
* Starts the client by connecting to the server and launching the Electron application
* @param {object} config server configuration
* @param {string} prefix http or https prefix
* @async
*/
async function startClient (config, prefix) {
try {
const serverUrl = `${prefix}${config.address}:${config.port}/config/`;
console.log(`Client: Connecting to server at ${serverUrl}`);
const configReturn = await getServerConfig(serverUrl);
console.log("Client: Successfully retrieved config from server");
// check environment for DISPLAY or WAYLAND_DISPLAY
const elecParams = ["js/electron.js"];
if (process.env.WAYLAND_DISPLAY) {
console.log(`Client: Using WAYLAND_DISPLAY=${process.env.WAYLAND_DISPLAY}`);
elecParams.push("--enable-features=UseOzonePlatform");
elecParams.push("--ozone-platform=wayland");
} else if (process.env.DISPLAY) {
console.log(`Client: Using DISPLAY=${process.env.DISPLAY}`);
} else {
fail("Error: Requires environment variable WAYLAND_DISPLAY or DISPLAY, none is provided.");
}
// Pass along the server config via an environment variable
const env = { ...process.env };
env.clientonly = true;
const options = { env: env };
configReturn.address = config.address;
configReturn.port = config.port;
configReturn.tls = config.tls;
env.config = JSON.stringify(configReturn);
// Spawn electron application
const electron = require("electron");
const child = require("node:child_process").spawn(electron, elecParams, options);
// Pipe all child process output to current stdout
child.stdout.on("data", function (buf) {
process.stdout.write(`Client: ${buf}`);
});
// Pipe all child process errors to current stderr
child.stderr.on("data", function (buf) {
process.stderr.write(`Client: ${buf}`);
});
child.on("error", function (err) {
process.stderr.write(`Client: ${err}`);
});
child.on("close", (code) => {
if (code !== 0) {
fail(`There is something wrong. The clientonly process exited with code ${code}.`);
}
});
} catch (reason) {
fail(`Unable to connect to server: (${reason})`);
}
}
// Main execution
const config = getServerParameters();
const prefix = config.tls ? "https://" : "http://";
// Validate port
if (config.port !== undefined && (isNaN(config.port) || config.port < 1 || config.port > 65535)) {
fail(`Invalid port number: ${config.port}. Port must be between 1 and 65535.`);
}
// Only start the client if a non-local server was provided and address/port are set
const LOCAL_ADDRESSES = ["localhost", "127.0.0.1", "::1", "::ffff:127.0.0.1"];
if (
config.address
&& config.port
&& !LOCAL_ADDRESSES.includes(config.address)
) {
startClient(config, prefix);
} else {
fail();
}

179
tests/e2e/clientonly_spec.js Normal file
View File

@@ -0,0 +1,179 @@
const { spawnSync, spawn } = require("node:child_process");
const delay = (time) => {
return new Promise((resolve) => setTimeout(resolve, time));
};
/**
* Run clientonly with given arguments and return result
* @param {string[]} args command line arguments
* @param {object} env environment variables to merge (replaces process.env)
* @returns {object} result with status and stderr
*/
const runClientOnly = (args = [], env = {}) => {
// Start with minimal env and merge provided env
const testEnv = {
PATH: process.env.PATH,
NODE_PATH: process.env.NODE_PATH,
...env
};
const result = spawnSync("node", ["clientonly/index.js", ...args], {
env: testEnv,
encoding: "utf-8",
timeout: 5000
});
return result;
};
describe("Clientonly parameter handling", () => {
describe("Missing parameters", () => {
it("should fail without any parameters", () => {
const result = runClientOnly();
expect(result.status).toBe(1);
expect(result.stderr).toContain("Usage:");
});
it("should fail with only address parameter", () => {
const result = runClientOnly(["--address", "192.168.1.10"]);
expect(result.status).toBe(1);
expect(result.stderr).toContain("Usage:");
});
it("should fail with only port parameter", () => {
const result = runClientOnly(["--port", "8080"]);
expect(result.status).toBe(1);
expect(result.stderr).toContain("Usage:");
});
});
describe("Local address rejection", () => {
it("should fail with localhost address", () => {
const result = runClientOnly(["--address", "localhost", "--port", "8080"]);
expect(result.status).toBe(1);
expect(result.stderr).toContain("Usage:");
});
it("should fail with 127.0.0.1 address", () => {
const result = runClientOnly(["--address", "127.0.0.1", "--port", "8080"]);
expect(result.status).toBe(1);
expect(result.stderr).toContain("Usage:");
});
it("should fail with ::1 address", () => {
const result = runClientOnly(["--address", "::1", "--port", "8080"]);
expect(result.status).toBe(1);
expect(result.stderr).toContain("Usage:");
});
it("should fail with ::ffff:127.0.0.1 address", () => {
const result = runClientOnly(["--address", "::ffff:127.0.0.1", "--port", "8080"]);
expect(result.status).toBe(1);
expect(result.stderr).toContain("Usage:");
});
});
describe("Port validation", () => {
it("should fail with port 0", () => {
const result = runClientOnly(["--address", "192.168.1.10", "--port", "0"]);
expect(result.status).toBe(1);
expect(result.stderr).toContain("Invalid port number");
});
it("should fail with negative port", () => {
const result = runClientOnly(["--address", "192.168.1.10", "--port", "-1"]);
expect(result.status).toBe(1);
expect(result.stderr).toContain("Invalid port number");
});
it("should fail with port above 65535", () => {
const result = runClientOnly(["--address", "192.168.1.10", "--port", "65536"]);
expect(result.status).toBe(1);
expect(result.stderr).toContain("Invalid port number");
});
it("should fail with non-numeric port", () => {
const result = runClientOnly(["--address", "192.168.1.10", "--port", "abc"]);
expect(result.status).toBe(1);
expect(result.stderr).toContain("Invalid port number");
});
it("should accept valid port 8080", () => {
const result = runClientOnly(["--address", "192.168.1.10", "--port", "8080"]);
// Should not fail on port validation (will fail on connection or display)
expect(result.stderr).not.toContain("Invalid port number");
});
it("should accept valid port 1", () => {
const result = runClientOnly(["--address", "192.168.1.10", "--port", "1"]);
expect(result.stderr).not.toContain("Invalid port number");
});
it("should accept valid port 65535", () => {
const result = runClientOnly(["--address", "192.168.1.10", "--port", "65535"]);
expect(result.stderr).not.toContain("Invalid port number");
});
});
describe("TLS flag parsing", () => {
// Note: These tests verify the flag is parsed, not the actual connection behavior
// Connection tests would timeout as they try to reach unreachable addresses
it("should not fail on port validation when using --use-tls", () => {
// Verify --use-tls doesn't interfere with other parameter parsing
const result = runClientOnly(["--address", "192.168.1.10", "--port", "443", "--use-tls"]);
expect(result.stderr).not.toContain("Invalid port number");
});
it("should accept --use-tls flag with valid parameters", () => {
const result = runClientOnly(["--address", "192.168.1.10", "--port", "443", "--use-tls"]);
// Should not fail on parameter parsing (will fail on connection or display)
expect(result.stderr).not.toContain("Usage:");
});
});
describe("Display environment check", () => {
it("should fail without DISPLAY or WAYLAND_DISPLAY when connecting to valid server", async () => {
// This test needs a running server to get past the connection phase
// Without DISPLAY, it should fail with display error
// For now, we just verify it fails (connection error comes first without server)
const result = runClientOnly(["--address", "192.168.1.10", "--port", "1"]);
// Either exits with code 1 or times out (null status means killed/timeout)
expect(result.status === 1 || result.status === null).toBe(true);
});
});
});
describe("Clientonly with running server", () => {
let serverProcess;
const testPort = 8081;
beforeAll(async () => {
process.env.MM_CONFIG_FILE = "tests/configs/default.js";
process.env.MM_PORT = testPort.toString();
serverProcess = spawn("node", ["--run", "server"], {
env: process.env,
detached: true
});
// Wait for server to start
await delay(2000);
});
afterAll(async () => {
if (serverProcess && serverProcess.pid) {
try {
process.kill(-serverProcess.pid);
} catch {
// Process may already be dead
}
}
});
it("should be able to fetch config from server", async () => {
const res = await fetch(`http://localhost:${testPort}/config/`);
expect(res.status).toBe(200);
const config = await res.json();
expect(config).toBeDefined();
expect(typeof config).toBe("object");
});
});