Reenable integration testing by matching the test files#138
Merged
Conversation
Contributor
|
Reviewed; no blockers found. |
cb1kenobi
approved these changes
May 12, 2026
| "lint:required": "oxlint --quiet .", | ||
| "test:integration": "HARPER_INTEGRATION_TEST_INSTALL_SCRIPT=dist/bin/harper.js harper-integration-test-run", | ||
| "test:integration:all": "npm run test:integration -- integrationTests/**/*.test.ts", | ||
| "test:integration:all": "npm run test:integration -- integrationTests/**/*.test.*s", |
Member
There was a problem hiding this comment.
In case you like verbosity 😄
Suggested change
| "test:integration:all": "npm run test:integration -- integrationTests/**/*.test.*s", | |
| "test:integration:all": "npm run test:integration -- integrationTests/**/*.test.{ts,js,mts,mjs}", |
Three distinct failures in tests that were never running because the glob only matched *.ts files: - cluster/fullyConnectedReplication, cluster/replicationLoad: imported targz from a relative path (../../core/integrationTests/utils/targz.ts) that no longer exists; moved to @harperfast/integration-testing which already exports it (matches how issue135 test does it) - analytics fixture: spawn() calls lacked the required name option (Harper's wrapped spawn enforces this to deduplicate long-lived processes); added unique name per child using Date.now() + index - cloneNode "three more nodes": waitForAvailableStatus only means the node is up, not that the full replication mesh has established; added a poll loop (same pattern as replicationTopology.test.mjs) that retries up to 20× with backoff before asserting all sockets are connected Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
DavidCockerill
approved these changes
May 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR re-enables integration tests that were silently skipped because the glob pattern
*.test.*sonly matched.tsfiles, excluding all.mjstest files. With the glob now correct, 8 previously-untouched tests ran for the first time and exposed three categories of failures:ERR_MODULE_NOT_FOUNDforcore/integrationTests/utils/targz.ts(fullyConnectedReplication,replicationLoad) — the file no longer exists;targzis now exported from@harperfast/integration-testing. Updated both imports to match howissue135-resource-search-after-restart.test.mjsalready does it.CpuWork endpoint not reachable(analytics.test.mjs) — Harper's wrappedspawn(seecore/security/jsLoader.ts:887) requires anameoption so it can deduplicate long-lived processes. The fixture called barespawn()which threw, returning ≥400 and causing the endpoint probe to fail. Added a uniquename: cpu-work-child-${runId}-${i}per child.AssertionError: connected(cloneNode— "should clone three more nodes") —waitForAvailableStatusonly means a node is reachable and bootstrapped, not that the full N×(N-1) replication mesh has finished establishing every socket. The fix adds a poll loop (same pattern asreplicationTopology.test.mjs:100–140) that retries up to 20× with backoff before assertingsocket.connected.Where to look
Low confidence / needs attention: The cloneNode socket-connected polling is the highest-risk change. If the cluster genuinely fails to mesh (rather than just being slow), the retry will exhaust and throw a detailed JSON dump — but it's worth checking that the 20×backoff window (≈5 min total) is sufficient given CI runner load. The existing
replicationTopologytest uses only 10 retries with smaller backoff; the clone test starts 3 additional nodes sequentially so needs more runway.Straightforward: The targz import changes are mechanical renames. The spawn
namefix is self-contained to the analytics fixture.Related
Addresses failures introduced when #137 fixed the glob to match
.mjsfiles.Generated by Claude Sonnet 4.6 (1M context)