From 9d4e8439297b6121ee25a215cd116133bd5ef137 Mon Sep 17 00:00:00 2001 From: Bryan English Date: Mon, 26 Oct 2026 15:02:04 -0400 Subject: [PATCH] test: restore the validate parity test and close coverage gaps The parity test deferred from the initial port is back: one shared battery of calls runs through lib/validate.js and through node's real internal validators and error constructors in a child under --expose-internals, and the outcomes must match down to the messages. outOfRangeError is exported again for the battery to construct. The coverage gaps: IPv6 authorities and portless Host headers on the http client (both scheme defaults, 80 and 443), the boolean, non-integer, and fallback arms of the OTLP attribute encoding, and the non-2xx collector response warning. The undici origin's scheme-default port stays uncovered: reaching it requires binding port 80 or 443. Signed-off-by: Bryan English --- lib/validate.js | 11 +--- test/common/validate-battery.js | 110 ++++++++++++++++++++++++++++++++ test/fixtures/parity-child.js | 29 +++++++++ test/test-exporter.js | 41 +++++++++++- test/test-flush-coverage.js | 47 +++++++++++++- test/test-http-client.js | 43 +++++++++++++ test/test-https.js | 65 +++++++++++++++++-- test/test-validate-parity.js | 43 +++++++++++++ 8 files changed, 371 insertions(+), 18 deletions(-) create mode 100644 test/common/validate-battery.js create mode 100644 test/fixtures/parity-child.js create mode 100644 test/test-validate-parity.js diff --git a/lib/validate.js b/lib/validate.js index a673f0f..5dd853e 100644 --- a/lib/validate.js +++ b/lib/validate.js @@ -3,14 +3,8 @@ // Faithful port of the small subset of Node.js core validators used by // this package (lib/internal/validators.js, lib/internal/errors.js), // matching core's error shapes (name, code, message) exactly. -// TODO(bengl): add back the parity test that proved it. Spawn a child -// with --expose-internals that runs one shared battery of calls through -// require('internal/validators') and the ERR_INVALID_ARG_TYPE, -// ERR_INVALID_ARG_VALUE, and ERR_OUT_OF_RANGE constructors from -// internal/errors, print the outcomes (thrown or not, and the thrown -// error's name, code, message) as JSON, and diff them against the same -// battery run through this module. When core changes these message -// formats, such a test is what catches the drift. +// test/test-validate-parity.js proves the parity by diffing this module +// against the real internals under --expose-internals. const { inspect } = require('node:util'); @@ -220,4 +214,5 @@ module.exports = { validateInteger, invalidArgTypeError, invalidArgValueError, + outOfRangeError, }; diff --git a/test/common/validate-battery.js b/test/common/validate-battery.js new file mode 100644 index 0000000..b3241d2 --- /dev/null +++ b/test/common/validate-battery.js @@ -0,0 +1,110 @@ +'use strict'; + +// The input battery for test-validate-parity.js. Both sides share +// this module, so the compared inputs can never drift apart. + +// A JSON-safe, human-readable label for a call. +function label(fn, args) { + const rendered = args.map((a) => + typeof a === 'string' ? `'${a}'` : String(a), + ); + return `${fn}(${rendered.join(', ')})`; +} + +// Calls fn with args and describes the outcome: nothing thrown, or the +// thrown error's name, code, and message. +function outcome(fn, args) { + try { + fn(...args); + return { thrown: false }; + } catch (err) { + return { + thrown: true, + name: err.name, + code: err.code, + message: err.message, + }; + } +} + +const kValidatorCases = [ + ['validateString', [123, 'x']], + ['validateString', [undefined, 'x']], + ['validateString', [null, 'x']], + ['validateString', [true, 'x']], + ['validateString', [Symbol('s'), 'x']], + ['validateString', [{ a: 1 }, 'x']], + ['validateString', [['a'], 'x']], + ['validateString', [() => {}, 'x']], + ['validateString', ['ok', 'x']], + ['validateObject', [null, 'x']], + ['validateObject', [[1, 2], 'x']], + ['validateObject', ['str', 'x']], + ['validateObject', [123, 'x']], + ['validateObject', [() => {}, 'x']], + ['validateObject', [{}, 'x']], + ['validateInteger', [0, 'x', 1]], + ['validateInteger', ['abc', 'x', 1]], + ['validateInteger', [1.5, 'x', 1]], + ['validateInteger', [NaN, 'x', 1]], + ['validateInteger', [5, 'x', 1]], + ['validateInteger', [9007199254740999, 'x', 1]], +]; + +// Error-constructor cases: impl.fn(...args) must return the error object. +const kErrorCases = [ + [ + 'invalidArgTypeError', + ['options.filter', ['string', 'null', 'undefined'], 42], + ], + ['invalidArgTypeError', ['options.endpoint', ['string'], 123]], + ['invalidArgTypeError', ['options.maxBufferSize', ['integer'], 'abc']], + ['invalidArgTypeError', ['options.foo', ['Object'], []]], + [ + 'invalidArgTypeError', + ['first argument', ['string', 'null', 'undefined'], 42], + ], + [ + 'invalidArgValueError', + ['options.endpoint', 'not-a-valid-url', 'must be a valid URL'], + ], + [ + 'invalidArgValueError', + ['options.serviceName', '', 'must be a non-empty string'], + ], + ['invalidArgValueError', ['options.maxBufferSize', 0]], + ['outOfRangeError', ['x', '>= 1 && <= 100', 0]], + ['outOfRangeError', ['n', 'an integer', 10000000000]], +]; + +// Runs the battery through impl: either this package's lib/validate.js +// exports or wrappers around core's internal error constructors. +function runBattery(impl) { + const results = []; + + for (const [name, args] of kValidatorCases) { + results.push({ + call: label(name, args), + outcome: outcome(impl[name], args), + }); + } + + for (const [name, args] of kErrorCases) { + const err = impl[name](...args); + results.push({ + call: label(name, args), + outcome: { + thrown: true, + name: err.name, + code: err.code, + message: err.message, + }, + }); + } + + return results; +} + +module.exports = { + runBattery, +}; diff --git a/test/fixtures/parity-child.js b/test/fixtures/parity-child.js new file mode 100644 index 0000000..b310ee7 --- /dev/null +++ b/test/fixtures/parity-child.js @@ -0,0 +1,29 @@ +'use strict'; + +// Spawned with --expose-internals and the battery module in +// BATTERY_MODULE: runs the battery through core's real internals and +// prints the outcomes as JSON for the parent to compare. + +const { + validateString, + validateObject, + validateInteger, +} = require('internal/validators'); +const { + codes: { ERR_INVALID_ARG_TYPE, ERR_INVALID_ARG_VALUE, ERR_OUT_OF_RANGE }, +} = require('internal/errors'); +const { runBattery } = require(process.env.BATTERY_MODULE); + +const impl = { + validateString, + validateObject, + validateInteger, + invalidArgTypeError: (name, expected, actual) => + new ERR_INVALID_ARG_TYPE(name, expected, actual), + invalidArgValueError: (name, value, reason) => + new ERR_INVALID_ARG_VALUE(name, value, reason), + outOfRangeError: (str, range, input) => + new ERR_OUT_OF_RANGE(str, range, input), +}; + +process.stdout.write(JSON.stringify(runBattery(impl))); diff --git a/test/test-exporter.js b/test/test-exporter.js index 210590a..fec14ed 100644 --- a/test/test-exporter.js +++ b/test/test-exporter.js @@ -3,7 +3,13 @@ const assert = require('node:assert'); const http = require('node:http'); const { describe, it } = require('node:test'); -const { fixture, spawnMicrotel } = require('./common/otel'); +const { flush } = require('../lib/flush'); +const { + fixture, + httpGet, + startHTTPServer, + spawnMicrotel, +} = require('./common/otel'); // This test verifies exporter error handling and process lifecycle behavior. // Tracing is activated in the child processes through the public init() API; @@ -11,6 +17,39 @@ const { fixture, spawnMicrotel } = require('./common/otel'); // argv. describe('otel exporter behavior', () => { + it('warns when the collector responds non-2xx', async () => { + // An in-process collector that rejects every export with a 500. The + // spans are dropped, and a throttled OTelExportWarning reports it. + const collector = http.createServer((req, res) => { + res.writeHead(500); + res.end(); + }); + await new Promise((resolve) => collector.listen(0, '127.0.0.1', resolve)); + + const warningPromise = new Promise((resolve) => { + process.on('warning', function onWarning(w) { + if (w.name === 'OTelExportWarning') { + process.removeListener('warning', onWarning); + resolve(w); + } + }); + }); + + const { start } = require('../lib/core'); + start({ endpoint: `http://127.0.0.1:${collector.address().port}` }); + + const target = await startHTTPServer(); + await httpGet(target.port, '/x'); + flush(); + + const warning = await warningPromise; + + await collector.close(); + await target.close(); + + assert.match(warning.message, /HTTP 500/); + }); + it('does not crash when collector is unreachable', async () => { // The child traces a request against its own server and flushes to a // dead endpoint. The export fails, but the child exits normally. diff --git a/test/test-flush-coverage.js b/test/test-flush-coverage.js index c183f58..2384511 100644 --- a/test/test-flush-coverage.js +++ b/test/test-flush-coverage.js @@ -5,7 +5,7 @@ const { after, afterEach, before, describe, it } = require('node:test'); const { Span, SPAN_KIND_INTERNAL } = require('../lib/span'); const { addSpan, flush, resetCaches } = require('../lib/flush'); -const { startTracing, delay } = require('./common/otel'); +const { startTracing, getSpans, delay } = require('./common/otel'); describe('flush.js coverage', () => { let collector; @@ -22,12 +22,51 @@ describe('flush.js coverage', () => { }); it('flush is a no-op when buffer is empty', async () => { + const before = collector.requests.length; + flush(); // Wait for any potential export request. await delay(100); - assert.strictEqual(collector.requests.length, 0); + assert.strictEqual(collector.requests.length, before); + }); + + it('encodes boolean, double, and fallback attribute values', async () => { + // A span-like object covering the OTLP encodings the instrumentations + // never produce: booleans, non-integer numbers, and values of other + // types, which fall back to strings. + const span = { + name: 'mixed-values', + traceId: 'a'.repeat(32), + spanId: 'b'.repeat(16), + kind: 1, + startTime: 100, + endTime: 200, + getAttributes() { + return { + bool: true, + ratio: 1.5, + misc: null, + }; + }, + getEvents() { + return []; + }, + status: { code: 0, message: '' }, + }; + + addSpan(span); + flush(); + + const payload = await collector.next(); + const attrs = {}; + for (const a of getSpans(payload)[0].attributes) { + attrs[a.key] = a.value; + } + assert.deepStrictEqual(attrs.bool, { boolValue: true }); + assert.deepStrictEqual(attrs.ratio, { doubleValue: 1.5 }); + assert.deepStrictEqual(attrs.misc, { stringValue: 'null' }); }); it('flush handles spanToOtlp errors gracefully', async () => { @@ -95,6 +134,8 @@ describe('flush.js coverage', () => { }); it('resetCaches clears the span buffer', async () => { + const before = collector.requests.length; + // A span so the buffer is non-empty. const span = new Span('test', SPAN_KIND_INTERNAL); span.end(); @@ -106,6 +147,6 @@ describe('flush.js coverage', () => { // Wait for any potential export request. await delay(100); - assert.strictEqual(collector.requests.length, 0); + assert.strictEqual(collector.requests.length, before); }); }); diff --git a/test/test-http-client.js b/test/test-http-client.js index 571ccae..73db244 100644 --- a/test/test-http-client.js +++ b/test/test-http-client.js @@ -218,6 +218,49 @@ describe('otel HTTP client spans', () => { ); }); + it('parses IPv6 authorities and default ports from the Host header', async () => { + const target = await startHTTPServer((req, res) => { + res.writeHead(200); + res.end('ok'); + }); + + const seen = []; + const request = (headers, path) => + new Promise((resolve, reject) => { + const req = http.get( + { host: '127.0.0.1', port: target.port, path, headers }, + (res) => { + res.resume(); + res.on('end', resolve); + }, + ); + req.on('error', reject); + }); + + // A bracketed IPv6 authority: the port follows the closing bracket. + await request({ Host: `[::1]:9999` }, '/ipv6'); + // A portless Host header means the scheme default. + await request({ Host: 'example.test' }, '/default-port'); + + const spans = await flushAndCollect(collector); + await target.close(); + + const v6 = getClientSpan(spans, '/ipv6'); + assert.ok(v6, 'Expected a client span for the IPv6 request'); + assert.partialDeepStrictEqual(spanAttrs(v6), { + 'server.port': '9999', + 'url.full': 'http://[::1]:9999/ipv6', + }); + + const def = getClientSpan(spans, '/default-port'); + assert.ok(def, 'Expected a client span for the portless Host request'); + assert.strictEqual( + spanAttrs(def)['server.port'], + '80', + 'A portless Host header must yield the scheme default', + ); + }); + it('creates error span for HTTP client connection refused', async () => { await httpGet(1, '/fail').catch(() => {}); diff --git a/test/test-https.js b/test/test-https.js index ba735e1..0e66c1f 100644 --- a/test/test-https.js +++ b/test/test-https.js @@ -4,15 +4,26 @@ const assert = require('node:assert'); const fs = require('node:fs'); const path = require('node:path'); const https = require('node:https'); -const { describe, it } = require('node:test'); -const { startTracing, flushAndCollect, spanAttrs } = require('./common/otel'); +const { after, before, describe, it } = require('node:test'); +const { + startTracing, + flushAndCollect, + getClientSpan, + spanAttrs, +} = require('./common/otel'); const fixturesDir = path.join(__dirname, 'fixtures'); describe('otel HTTPS server spans', () => { - it('marks the url.scheme attribute as https for TLS requests', async () => { - const collector = await startTracing(); + let collector; + + before(async () => { + collector = await startTracing(); + }); + after(() => collector.close()); + + it('yields the 443 default port for a portless https Host header', async () => { const server = https.createServer( { key: fs.readFileSync(path.join(fixturesDir, 'https-key.pem')), @@ -31,9 +42,10 @@ describe('otel HTTPS server spans', () => { { host: '127.0.0.1', port: server.address().port, - path: '/secure', + path: '/default-port', rejectUnauthorized: false, agent: false, + headers: { Host: 'example.test' }, }, (res) => { res.resume(); @@ -44,8 +56,49 @@ describe('otel HTTPS server spans', () => { }); const spans = await flushAndCollect(collector); + await new Promise((resolve) => server.close(resolve)); - await collector.close(); + const clientSpan = getClientSpan(spans, '/default-port'); + assert.ok(clientSpan, 'Expected a client span'); + assert.strictEqual( + spanAttrs(clientSpan)['server.port'], + '443', + 'A portless https Host header must yield 443', + ); + }); + + it('marks the url.scheme attribute as https for TLS requests', async () => { + const server = https.createServer( + { + key: fs.readFileSync(path.join(fixturesDir, 'https-key.pem')), + cert: fs.readFileSync(path.join(fixturesDir, 'https-cert.pem')), + }, + (req, res) => { + res.writeHead(200); + res.end('ok'); + }, + ); + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + + await new Promise((resolve, reject) => { + https + .get( + { + host: '127.0.0.1', + port: server.address().port, + path: '/secure', + rejectUnauthorized: false, + agent: false, + }, + (res) => { + res.resume(); + res.on('end', resolve); + }, + ) + .on('error', reject); + }); + + const spans = await flushAndCollect(collector); await new Promise((resolve) => server.close(resolve)); const serverSpan = spans.find((s) => s.kind === 2); // SPAN_KIND_SERVER diff --git a/test/test-validate-parity.js b/test/test-validate-parity.js new file mode 100644 index 0000000..70931a2 --- /dev/null +++ b/test/test-validate-parity.js @@ -0,0 +1,43 @@ +'use strict'; + +const assert = require('node:assert'); +const path = require('node:path'); +const { describe, it } = require('node:test'); + +const validate = require('../lib/validate'); +const { runBattery } = require('./common/validate-battery'); +const { fixture, spawnMicrotel } = require('./common/otel'); + +// lib/validate.js ports a subset of core's validators and error message +// builders. This test proves the port: the same battery runs through the +// port and through the real internals (a child under --expose-internals), +// and the outcomes must match down to the error messages. If core changes +// these formats, this test fails and the port needs updating. + +describe('validate.js parity with node core', () => { + it('matches internal/validators and internal/errors exactly', async () => { + const { code, stdout, stderr } = await spawnMicrotel( + ['--expose-internals', fixture('parity-child.js')], + { + BATTERY_MODULE: path.join(__dirname, 'common', 'validate-battery.js'), + }, + ); + + assert.strictEqual(code, 0, `child failed: ${stderr}`); + + const core = JSON.parse(stdout); + const ported = runBattery(validate); + + // Normalize through JSON so values like undefined compare cleanly. + const portedJson = JSON.parse(JSON.stringify(ported)); + + assert.strictEqual(core.length, portedJson.length); + for (let i = 0; i < portedJson.length; i++) { + assert.deepStrictEqual( + core[i], + portedJson[i], + `${portedJson[i].call} diverges from core`, + ); + } + }); +});