Skip to content

test: add buildPolicySettings to the db mock in application.real.test.ts - #212

Merged
AminDhouib merged 1 commit into
canaryfrom
fix/build-policy-db-mock
Sep 11, 2026
Merged

AminDhouib merged 1 commit into
canaryfrom
fix/build-policy-db-mock

Conversation

@AminDhouib

Copy link
Copy Markdown
Member

Fixes the pr-check (test) failure that has been red on every PR targeting
canary since #209. One file, one added namespace in a test mock, eleven lines
of which nine are the comment.

The failure

Five tests in apps/dokploy/__test__/deploy/application.real.test.ts:

TypeError: Cannot read properties of undefined (reading 'findFirst')
 ❯ findBuildPolicySettings packages/server/src/services/build-policy/settings.ts:18
 ❯ previewBuildPolicyDecision packages/server/src/services/build-policy/resolve.ts:57
 ❯ resolveBuildPolicy         packages/server/src/services/build-policy/resolve.ts:107
 ❯ planApplicationBuild       packages/server/src/services/build-policy/apply.ts:243
 ❯ deployApplication          packages/server/src/services/application.ts:211

That file mocks @dokploy/server/db with a hand-written query namespace
(application.real.test.ts:15-53) listing applications, deployHook,
domains, patch and member. #209 added a deploy-path read of
db.query.buildPolicySettings, which is undefined there.

The fifth failure is the same cause wearing a different hat: the TypeError is
raised inside deployApplication's own try, so the deployment log came back
carrying ❌ [build-policy] cannot read properties of undefined (reading 'findfirst') where the test expected the deploy's own error text.

The fix, and why one namespace is enough

An unstubbed vi.fn() resolves undefined; findBuildPolicySettings maps that
to null (settings.ts:21, row ?? null); decideBuildPolicy reads a null
settings row as policy-off and returns local / not_enforced
(policy.ts:65-68). So these tests go on asserting the upstream deploy
behaviour they asserted before #209, which is what they are for.

Nothing else in the module is reachable from here.
previewBuildPolicyDecision returns at resolve.ts:59-72 without touching
exclusions or break-glass when the settings row is absent, and
resolveBuildPolicy writes no audit row for a local decision. The only other
build-policy read of this namespace is isBuildPolicyEnforcedAnywhere
(settings.ts:52), which the same mock entry would satisfy if a future test
reached it.

Evidence this is #209's, not the fork's baseline

Three runs of pull-request.yml:

Run Head Has #209 Tests
34048304936 b0161304, 2026-09-06 no 1912 passed, 1 skipped, 0 failed
34543739811 6d27c886, the #209 head merged as b0cadcd yes 5 failed, 2309 passed, 1 skipped
34611157554 3b7e233 (#210, docs-only) yes the same 5

Why four review rounds missed it

The reviewers ran the suite locally on Windows and compared failing-test sets
against the merge base, which matched exactly in both directions.
S/track2/w6-review-2.md:78 lists this same file as item 5 of the files that
already fail on that host, for an unrelated reason: Command failed: mkdir -p C:\…. It was red before and after, so a set comparison could not show that #209
had changed why it is red. On Linux CI the Windows path problem does not exist
and the build-policy reason surfaces instead.

The general lesson is worth keeping: a local baseline that already fails a file
proves nothing about that file.

Scope

Test-only. No production code, schema, workflow or dependency changes. Context:
#210 and docs/build-once-rollout-runbook.md §5 G6.

These five tests have failed on every PR targeting canary since #209 with
"Cannot read properties of undefined (reading 'findFirst')". The file mocks
@dokploy/server/db with a hand-written query namespace listing only
applications, deployHook, domains, patch and member; build-policy reaches
db.query.buildPolicySettings on the deploy path, through planApplicationBuild
-> resolveBuildPolicy -> previewBuildPolicyDecision -> findBuildPolicySettings
(services/build-policy/settings.ts:18).

Adds that one namespace. An unstubbed vi.fn() resolves undefined, which
findBuildPolicySettings maps to null, which is the policy-off answer, so these
tests keep asserting upstream deploy behaviour exactly as they did before #209.

Nothing else in build-policy is reachable here: previewBuildPolicyDecision
returns before touching exclusions or break-glass when the settings row is
absent (resolve.ts:59-72).

The four review rounds could not see this. The reviewers compared local
failing-test sets against the merge base and they matched, because this same
file already failed on their Windows host for an unrelated reason
(S/track2/w6-review-2.md:78, "Command failed: mkdir -p C:\"). On Linux CI
that problem does not exist and the file fails for the build-policy reason
instead.
@AminDhouib

Copy link
Copy Markdown
Member Author

Green. Run 34615190789, job pr-check (test):

Test Files  214 passed (214)
Tests       2314 passed | 1 skipped (2315)

Zero failures, and __test__/deploy/application.real.test.ts now reports ✓ (6 tests | 1 skipped). Against #210's run 34611157554 on the same base — 213 passed / 1 failed files, 2309 passed / 5 failed / 1 skipped tests — the recovered set is exactly the five this PR targets. build and typecheck also pass.

@AminDhouib
AminDhouib merged commit e92e4ad into canary Sep 11, 2026
3 checks passed
@AminDhouib
AminDhouib deleted the fix/build-policy-db-mock branch September 11, 2026 15:40
AminDhouib added a commit that referenced this pull request Sep 11, 2026
…ming

Three corrections after #211 and #212 landed on canary.

The republish point in section 2.1 was overstated. Step 1 reads the rollback
digest off the running service on the host, not off a tag, and a republish adds
a GHCR version rather than rewriting the old manifest, so a merge to canary
cannot invalidate it. Rewritten to say what actually follows: pin the digest you
are rolling to, and keep the docker save, because each republish leaves the
previous build untagged and that is what retention sweeps collect. Also records
that the fork is in neither Dokploy CSV, so a merge to canary redeploys nothing.

Section 3.1 and 3.2 now account for canary having moved twice since b0cadcd.
Both follow-ups are runtime-neutral - a CI trigger and a file under __test__ -
and either build is a valid rollout target. Section 3.2 now shows how to resolve
the current head's digest, and keeps the b0cadcd digest for anyone who would
rather roll the reviewed commit and nothing else.

Section 5 G6 and section 7 step 0 record that the test regression is fixed:
#212 (e92e4ad), run 34615190789, 214 files passed, 2314 passed / 1 skipped /
0 failed.
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