Skip to content

Upgrade Harper to v5: add integration tests, CI workflow, and dev tooling#48

Open
BboyAkers wants to merge 5 commits into
mainfrom
v5-upgrade
Open

Upgrade Harper to v5: add integration tests, CI workflow, and dev tooling#48
BboyAkers wants to merge 5 commits into
mainfrom
v5-upgrade

Conversation

@BboyAkers

Copy link
Copy Markdown
Member

Summary

  • Install harper@^5.0.28 and @harperfast/integration-testing@^0.4.0 as devDependencies
  • Add TypeScript devDependency and tsconfig.json
  • Add integrationTests/metrics.test.ts covering:
    • Harper starts successfully
    • GET /prometheus_exporter/metrics returns HTTP 200 with Prometheus/OpenMetrics text
    • Response content-type is text/plain or application/openmetrics-text
    • Default Node.js process metrics (from prom-client collectDefaultMetrics()) are present
    • /metrics/fast path returns 200
    • PrometheusExporterSettings table is accessible via REST
    • forceAuthorization setting is initialized
  • Apply harperBinPath fix in test setup (workaround for ERR_PACKAGE_PATH_NOT_EXPORTEDharper's exports map only exposes ".", so harper/dist/bin/harper.js is not resolvable via the harness auto-resolve path)
  • Add .github/workflows/integration-tests.yml (Node matrix 22/24/26, pinned actions to commit hashes)
  • Regenerate package-lock.json with --os=linux --cpu=x64 --include=optional so bufferutil, utf-8-validate, and node-gyp-build are recorded for Linux CI
  • Un-ignore package-lock.json (was in .gitignore; CI needs it for npm ci)
  • Update package.json author branding: HarperDB, Inc.Harper
  • Update README note: exporter supports Harper v5 in addition to v4.2+

Migration items applied / N/A

Item Status
harperdbharper import swap N/A — harperdb/harper is a runtime-provided global, not a listed dependency
Table.get() returns plain frozen records Already handled — recordProp compat shim added in commit 73e2fa6
wasLoadedFromSource()target.loadedFromSource N/A — no cache source pattern
Blob.save()saveBeforeCommit N/A — no blob storage
Process spawning whitelisting N/A — no child process spawning
Transaction context N/A — no explicit transaction management

Known issues / Notes

  • Local tests fail with EADDRNOTAVAIL: macOS loopback aliases are not configured on this machine (only 127.0.0.1 exists, and the integration harness binds Harper to 127.0.0.2+). This is environmental — CI runs on ubuntu-latest which supports the full 127.0.0.0/8 range. CI is the gate.
  • harperBinPath workaround: upstream issue — harper package should export its bin path or the harness should resolve via the package root instead of the blocked deep subpath.

npm scope

The package name is @harperdb/prometheus-exporter. If this should move to a current Harper npm scope (@harperfast/ or similar), that is a manual step — flagged for the team per the upgrade plan (§11.1).

Test plan

  • CI passes on Node 22, 24, and 26
  • GET /prometheus_exporter/metrics returns valid Prometheus/OpenMetrics text
  • PrometheusExporterSettings table initializes correctly on first boot

🤖 Generated with Claude Code

- Install harper@5.0.28 and @harperfast/integration-testing as devDependencies
- Add TypeScript devDependency and tsconfig.json
- Add integrationTests/metrics.test.ts covering the /metrics endpoint,
  content-type, default Node.js process metrics, and PrometheusExporterSettings
  table initialization; apply harperBinPath fix (ERR_PACKAGE_PATH_NOT_EXPORTED)
- Add .github/workflows/integration-tests.yml (Node matrix 22/24/26, pinned actions)
- Regenerate package-lock.json with --os=linux --cpu=x64 --include=optional so
  bufferutil/utf-8-validate/node-gyp-build are recorded for Linux CI
- Un-ignore package-lock.json (was in .gitignore; CI needs it for npm ci)
- Update package.json author branding HarperDB, Inc. -> Harper
- Update README note: exporter supports Harper v5 in addition to v4.2+

Migration items N/A: no harperdb imports (runtime global); no Table.get() return
shape issues; no Blob.save(); no wasLoadedFromSource(); v5 plain-record access
fix was already applied in commit 73e2fa6 (recordProp compat shim).

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@gemini-code-assist

Copy link
Copy Markdown

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

BboyAkers and others added 2 commits June 8, 2026 16:29
Harper returns the Prometheus string as application/json when no explicit
Accept header is sent. Update the content-type test to accept that, and add
an extractMetricsText() helper that unwraps JSON-encoded strings before
checking for Prometheus metric comments.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
// harper's `exports` only exposes ".", so 'harper/dist/bin/harper.js' is not resolvable.
// Resolve the CLI from the exported main entry and pass it explicitly.
const require = createRequire(import.meta.url);
const harperBinPath = resolve(dirname(require.resolve('harper')), 'bin/harper.js');

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fragile bin path resolutionrequire.resolve('harper') returns the package's main entry (dist/index.js), so dirname() gives dist/. The resolved path dist/bin/harper.js happens to match harper 5.0.28's bin field by coincidence. If harper's main entry ever moves to a different subdirectory (or to the package root), dirname() will produce the wrong base and the path will silently point nowhere, crashing every test run with a cryptic ENOENT.

A more resilient approach resolves from the package root directly:

// Use the package.json to anchor at the package root
const harperRoot = dirname(require.resolve('harper/package.json'));
const harperBinPath = resolve(harperRoot, 'dist/bin/harper.js');

(Or check whether setupHarperWithFixture already accepts a package name so the bin path lookup can live in the integration-testing library.)

Comment thread integrationTests/metrics.test.ts Outdated
const ct = res.headers.get('content-type') ?? '';
if (ct.includes('application/json')) {
const body = await res.json() as unknown;
return typeof body === 'string' ? body : JSON.stringify(body);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silent swallow of non-string JSON body — when Harper responds with Content-Type: application/json but the parsed body is not a string (e.g. an error object like {"error": "..."}), extractMetricsText returns JSON.stringify(body), which looks like {"error":"..."}. The calling test then asserts text.includes('# HELP'), which fails with the message 'Response should contain Prometheus metric comments' — giving no indication that an error document was returned instead of metrics.

Consider throwing instead so the failure message is actionable:

if (typeof body !== 'string') {
  throw new Error(`Expected string from JSON-wrapped metrics response, got: ${JSON.stringify(body)}`);
}
return body;

Comment thread integrationTests/metrics.test.ts Outdated
void test('Harper starts successfully', async () => {
const res = await authFetch(ctx, '/');
ok(
[200, 400, 404].includes(res.status),

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

400 accepted as 'starts successfully' — including 400 Bad Request in the success set means this test passes even when Harper is running but completely misconfigured. In that state every subsequent test would run against a broken Harper and produce misleading failures, rather than this health-check surfacing the root cause.

If Harper's root / legitimately returns 400 for a well-formed GET (e.g. it only accepts POST), document why and add a comment; otherwise tighten the check:

ok(
  [200, 404].includes(res.status),
  `Unexpected status ${res.status}`,
);

Comment thread integrationTests/metrics.test.ts Outdated
ct.includes('text/plain') ||
ct.includes('application/openmetrics-text') ||
ct.includes('application/json') ||
ct.includes('text/'),

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Content-type assertion is over-broad — the final ct.includes('text/') arm matches any text/* media type, including text/html (an error page). If Harper ever returns an HTML error response with HTTP 200, this test passes while the metrics endpoint is actually broken.

Additionally, ct.includes('text/plain') is completely redundant: every text/plain value is already caught by ct.includes('text/'), so that branch is dead code.

Tighten to the three types that are actually expected:

ok(
  ct.includes('text/plain') ||
    ct.includes('application/openmetrics-text') ||
    ct.includes('application/json'),
  `Unexpected content-type: ${ct}`,
);

Comment thread .github/workflows/integration-tests.yml Outdated

steps:
- name: Checkout code
uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary checkout — the generate-node-version-matrix job only runs a shell if/else that inspects the NODE_VER environment variable. It never reads any file from the repository. The actions/checkout step adds a few seconds to every CI run (clone + LFS + submodule overhead) and is not needed here.

Remove the checkout step from this job to keep it lean.

BboyAkers and others added 2 commits June 11, 2026 13:11
- Anchor harperBinPath at package root via harper/package.json instead of main-entry dirname
- Throw on non-string JSON body in extractMetricsText instead of silently stringifying
- Remove HTTP 400 from accepted status set in 'Harper starts successfully' test
- Remove catch-all text/ and dead text/plain arms from content-type assertion
- Remove unnecessary checkout step from generate-node-version-matrix CI job

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant