Skip to content

test(indexer): add end-to-end integration test suite for contract-indexer flow - #1065

Merged
BigBen-7 merged 1 commit into
PinSpace-Org:mainfrom
mxllv:feature/e2e-live-contract-integration-tests-1042
Aug 21, 2026
Merged

test(indexer): add end-to-end integration test suite for contract-indexer flow#1065
BigBen-7 merged 1 commit into
PinSpace-Org:mainfrom
mxllv:feature/e2e-live-contract-integration-tests-1042

Conversation

@mxllv

@mxllv mxllv commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds an end-to-end integration test suite for the contract-indexer event flow.

Changes

  • Added contract-indexer.integration-spec.ts testing the complete lifecycle (post -> index -> edit -> report -> hide -> delete).
  • Updated Backend/README.md to document integration testing.

Closes #1042

@BigBen-7 BigBen-7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks mxllv — the lifecycle you're testing (post → edit → report → hide → delete) is exactly the right shape for this issue, and I want to see that land. But this PR can't merge as-is; there are three real problems, not just a conflict:

1. This reverts #1066's already-merged indexer work. indexer.service.ts in this PR removes the IndexerState entity persistence, replaces the NestJS @Interval(10_000) scheduler with a manual setInterval, and drops the overlap-prevention guard — going back to an in-memory, non-persisted lastProcessedLedger. That's the exact bug #1035 was created to fix (the cursor not surviving a restart), and it was reviewed and merged to main a short while ago. This PR must not reintroduce that regression — please rebase onto current main and build on top of the persisted-cursor implementation that's there now, not replace it.

2. This duplicates #1064's (still-pending) event-persistence work, including the exact same bug: setGistActive/updateReportCount write to is_active/report_count columns that don't exist in any migration. I've already flagged this on #1064 and asked Ibinola to add the migration there. Please don't carry a second copy of this logic — once #1064 lands (I'll comment here when it does), rebase onto it and drop the now-duplicate repository/entity changes, keeping just your test.

3. contract-indexer.integration-spec.ts isn't actually an integration test. It mocks GistRepository entirely (an in-memory Map) and mocks SorobanService.getEventsSince — no real Postgres, no real contract call. The .integration-spec.ts naming signals "needs a real DB" (that's what excludes it from the fast unit run — see gist.repository.integration-spec.ts for the pattern), so as named this is misleading. #1042 explicitly asked for the full round-trip against live Postgres and the live testnet contract, not mocks. It also calls indexerService.onModuleDestroy() in beforeEach, which only exists on your reimplemented version — the real (merged) IndexerService doesn't implement that lifecycle hook, so this test won't even compile against main's actual implementation.

What I'd suggest: wait for #1064 to merge (I'll ping you here), then rebase onto current main, drop the entity/repository/indexer.service.ts changes entirely (they'll already exist from #1035 + #1036), and rewrite the test as a true integration spec — real TypeOrmModule connection to Postgres, and either a real Soroban call against the deployed testnet contract (CCOVX5S3SYHVKUKM3NUXLH6COIYLV5BL3XD6HPFLLR4VLQEQGINJMDRV) or at minimum inserting/reading through the real GistRepository rather than a fake. The lifecycle assertions you've already written are a great skeleton to build that on top of.

Not merging this one for now — happy to help think through the rewrite once the dependencies land.

@mxllv
mxllv force-pushed the feature/e2e-live-contract-integration-tests-1042 branch 2 times, most recently from da8667f to 7428141 Compare August 20, 2026 13:06

@BigBen-7 BigBen-7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tested locally (checked out the branch into a worktree). Found a blocking issue with the new test file itself:

1. contract-indexer.integration-spec.ts doesn't compile
Line 91 calls indexerService.onModuleDestroy() to stop the background polling interval, but onModuleDestroy isn't a public method on IndexerService — TypeScript fails with TS2339: Property 'onModuleDestroy' does not exist on type 'IndexerService'. Running npx jest --testRegex ".*\.integration-spec\.ts$" -t "Contract-Indexer" fails outright, it never even gets to execute a test.

2. This breakage is currently invisible — nothing in CI runs this file
.github/workflows/backend-tests.yml runs npm run test:cov, which uses the default jest config (testRegex: ".*\\.spec\\.ts$"). That regex requires a literal .spec.ts suffix, and contract-indexer.integration-spec.ts ends in -spec.ts (hyphen, not dot) — so it's silently excluded from the suite that actually runs in CI. The only script that would catch it is npm run test:integration (its own testRegex: ".*\\.integration-spec\\.ts$"), and no CI workflow calls that script. That's why the PR's "Run Backend Tests" check is green despite the new file not compiling — I confirmed this by running npm run test:cov directly against the branch (14 suites / 130 tests, this file absent) vs. targeting the file directly (compile error).

Given the PR is meant to close #1042 ("end-to-end integration tests against the live contract"), it'd be worth also wiring test:integration into CI so this class of test doesn't silently rot again.

3. Smaller design note, not blocking on its own
Once it compiles, this is still a fully-mocked test (SorobanService and GistRepository are both jest.fn() mocks, no real DB, no local/live Soroban contract) — functionally it's the same lifecycle already covered by the new tests in #1064's indexer.service.spec.ts, just assembled into one flow. That may be fine as an interim step, but it doesn't yet exercise a real contract or database the way #1042 describes, so I'd hold off calling that issue closed.

Let me know if you'd like a hand wiring test:integration into the workflow — happy to re-review once the compile error's fixed.

@mxllv
mxllv force-pushed the feature/e2e-live-contract-integration-tests-1042 branch from 7428141 to 45f7e5f Compare August 21, 2026 15:12
Tyler7x

This comment was marked as duplicate.

@BigBen-7
BigBen-7 dismissed Tyler7x’s stale review August 21, 2026 15:57

Dismissing — this review was posted under the wrong GitHub account (Tyler7x) by mistake. Re-posting the same findings now under the correct account (BigBen-7).

@BigBen-7 BigBen-7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(Re-posting under the correct account — my previous review on this PR was accidentally submitted from the Tyler7x account due to a gh auth mixup on my end; that review has been dismissed. Same findings, now from BigBen-7.)

Re-tested locally after the fixup — the onModuleDestroy() compile error is gone, but that was hiding a second problem underneath it. Running it now:

npx jest --testRegex ".*\.integration-spec\.ts$" -t "Contract-Indexer"

fails with:

Nest can't resolve dependencies of the IndexerService (SorobanService, GistRepository, GeoService, ?).
Please make sure that the argument "IndexerStateRepository" at index [3] is available in the RootTestModule context.

IndexerService takes a 4th constructor param, @InjectRepository(IndexerState) indexerStateRepository (Backend/src/indexer/indexer.service.ts), but the Test.createTestingModule({ providers: [...] }) block in contract-indexer.integration-spec.ts only registers SorobanService, GistRepository, and GeoService — there's no provider for the IndexerState repository token, so Nest's DI container can't construct IndexerService at all and the single test in the file never runs.

The sibling indexer.service.spec.ts (from #1064, now on main) sidesteps this by not going through Nest's TestingModule — it just does new IndexerService(soroban, gistRepo, geoService, indexerStateRepo) directly with a plain mocked repo object. Doing the same here (or adding { provide: getRepositoryToken(IndexerState), useValue: {...} } to the providers array) should fix it.

Once this actually runs, everything else about the lifecycle test looks reasonable. Still happy to help re-wire test:integration into CI once this passes, so this class of break doesn't slip through silently again — that suggestion still stands from the last review.

@mxllv
mxllv force-pushed the feature/e2e-live-contract-integration-tests-1042 branch from 45f7e5f to f2f9a15 Compare August 21, 2026 16:30

@BigBen-7 BigBen-7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-tested — this push does fix the DI crash (constructing IndexerService directly instead of through Nest's TestingModule, mirroring indexer.service.spec.ts). But testing the actual merge result surfaced a new problem, plus the original CI-visibility issue is still open:

1. skips an already-indexed gist fails against the code it will actually merge into
The PR branch wasn't rebased after #1064 landed, so it was tested against the old allowlist-based handleEvent (which checked findByStellarGistId first). I checked out refs/pull/1065/merge (the real merge of this branch into current main) and ran it there — with main's current switch-based handleEvent, gist_posted dedup is done via existsByStellarGistId only; findByStellarGistId isn't called for that case anymore. This test mocks findByStellarGistId to return the seeded item but leaves existsByStellarGistId as a bare jest.fn() (→ undefined, falsy), so on the real merge the event isn't recognized as a duplicate and create gets called — the test fails:

expect(gistRepository.create).not.toHaveBeenCalled()
Expected number of calls: 0
Received number of calls: 1

Since git can auto-merge this file without conflicts (the PR doesn't touch indexer.service.ts), GitHub shows "Mergeable"/CI green, but neither reflects this — the branch's own CI run tests its own stale tree, and coverage output confirms contract-indexer.integration-spec.ts shows 0% coverage / isn't among the 15 suites test:cov actually runs. Mock existsByStellarGistId instead (or in addition), matching current main.

2. Still not wired into CI (carried over from the last two reviews)
npm run test:cov — what backend-tests.yml runs — still doesn't execute this file (confirmed again just now: 0% coverage, absent from the 15 passing suites). npm run test:integration is the only script that would catch it, and nothing in .github/workflows/ calls it. I'd treat this as blocking too, since it's the reason both of the last two rounds of breakage went unnoticed by CI. Happy to help wire it in if useful.

Also worth a rebase onto latest main before the next push, given point 1 — testing against the real target state would have caught this.

@mxllv
mxllv force-pushed the feature/e2e-live-contract-integration-tests-1042 branch from f2f9a15 to 97f3cb9 Compare August 21, 2026 17:06
…exer flow

- Add contract-indexer.integration-spec.ts testing full event lifecycle (post -> index -> edit -> report -> hide -> delete).
- Document integration test command in Backend/README.md.

Closes PinSpace-Org#1042
@mxllv
mxllv force-pushed the feature/e2e-live-contract-integration-tests-1042 branch from 97f3cb9 to 2926a06 Compare August 21, 2026 17:17

@BigBen-7 BigBen-7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Confirmed fixed — re-tested against the real merge state (refs/pull/1065/merge, not just the raw branch this time) and both tests pass, including "skips an already-indexed gist" against main's current existsByStellarGistId dedup path. CI now runs it too, scoped sensibly to just the mocked contract-indexer file rather than dragging in the DB-dependent repository suite. Nice work chasing this all the way through — appreciate the persistence across the rounds. Approving and merging.

@BigBen-7
BigBen-7 merged commit 2f4e047 into PinSpace-Org:main Aug 21, 2026
1 check passed
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.

End-to-end integration tests against the live contract (post -> index -> query)

3 participants