Skip to content

Commit 452acee

Browse files
test(ecs): pass the required onChangeFunction, and pin it with an inbound message
Correctness review of this branch found the only certain defects on it were inside its own regression test. The two `Engine({ entityContainer })` calls omitted `onChangeFunction`, which IEngineOptions declares as required. That is not cosmetic: engine/index.ts:310 is `options?.onChangeFunction(...)` — the optional chain guards `options`, not the member — so with a partial options object the call is `undefined(...)`. Those engines threw on their first inbound message. Every test in both blocks passed because none of them ticked. Verified directly: WITH onChangeFunction -> update() resolved, component landed WITHOUT onChangeFunction -> update() THREW: options?.onChangeFunction is not a function no options at all -> update() resolved (the optional chain short-circuits) Added a test per block that feeds a real inbound PUT through a transport, because `onChangeFunction` is only invoked from the CRDT receive path — a bare update() with no transport never reaches it, so my first two attempts at this test were vacuous. Negative control: removing the option now fails 1 of 26 with that TypeError. Also rewrote the setup comment above the recycling block. It claimed the avatar tombstone had to be recorded before any recyclable scene number "or the avatar key is never reached" — true against an unfixed container, but on this branch `updateRemovedEntity` refuses the tombstone outright, so `removedEntities` never holds that key and the loop guard is never consulted. The tests were passing for a different reason than the comment gave. Now says what the setup is actually for: making them discriminate against unfixed source.
1 parent 3d4c50d commit 452acee

1 file changed

Lines changed: 39 additions & 9 deletions

File tree

test/ecs/reserved-entity-range.spec.ts

Lines changed: 39 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ import {
77
createEntityContainer
88
} from '../../packages/@dcl/ecs/src/engine/entity'
99
import { Schemas } from '../../packages/@dcl/ecs/src/schemas'
10+
import { ReadWriteByteBuffer } from '../../packages/@dcl/ecs/src/serialization/ByteBuffer'
11+
import { PutComponentOperation } from '../../packages/@dcl/ecs/src/serialization/crdt'
1012

1113
/**
1214
* The renderer owns entity NUMBERS [0, RESERVED_STATIC_ENTITIES) at every version:
@@ -28,18 +30,20 @@ describe('Reserved entity range ownership', () => {
2830
let entityContainer: IEntityContainer
2931
let avatarSlot: Entity
3032

31-
// Two preconditions are both required, and getting either wrong makes these tests vacuous.
32-
// First, a standing allocation deficit, so generateEntity() reaches its recycling loop
33-
// instead of short-circuiting to generateNewEntity(). Second, the avatar number must be
34-
// recorded BEFORE any recyclable scene number: `removedEntities` is a Map iterated in
35-
// insertion order and the loop returns the first eligible entry, so a scene number
36-
// recorded first is handed out and the avatar key is never reached.
33+
// This setup exists to make the tests DISCRIMINATE. Against a fixed container the
34+
// tombstone is refused outright, so neither precondition below has any effect — but both
35+
// are what make these tests fail against an unfixed one, and getting either wrong makes
36+
// them vacuous. First, a standing allocation deficit, so generateEntity() reaches its
37+
// recycling loop rather than short-circuiting to generateNewEntity(). Second, the avatar
38+
// number is recorded BEFORE any recyclable scene number: `removedEntities` is a Map
39+
// iterated in insertion order and the loop returns the first eligible entry, so a scene
40+
// number recorded first would be handed out and the avatar key never reached.
3741
beforeEach(() => {
3842
entityContainer = createEntityContainer()
3943
avatarSlot = EntityUtils.toEntityId(AVATAR_SLOT_NUMBER, 0)
4044
// 4 allocations, then 2 released -> counter 516, used 2 -> deficit of 2.
4145
const owned = Array.from({ length: 4 }, () => entityContainer.generateEntity())
42-
// Avatar tombstone FIRST, so its key leads the insertion order.
46+
// Avatar tombstone FIRST, so its key would lead the insertion order (see above).
4347
entityContainer.updateRemovedEntity(avatarSlot)
4448
entityContainer.removeEntity(owned[0])
4549
entityContainer.removeEntity(owned[1])
@@ -192,7 +196,10 @@ describe('Reserved entity range ownership', () => {
192196
let sceneEntity: Entity
193197

194198
beforeEach(() => {
195-
engine = Engine({ entityContainer: createEntityContainer({ reservedStaticEntities: 64 }) })
199+
engine = Engine({
200+
onChangeFunction: () => {},
201+
entityContainer: createEntityContainer({ reservedStaticEntities: 64 })
202+
})
196203
Owned = engine.defineComponent('test::owned', { value: Schemas.Int })
197204
// Number 100 is scene-owned under a bound of 64, but sits inside the default range.
198205
sceneEntity = EntityUtils.toEntityId(100, 0)
@@ -214,13 +221,36 @@ describe('Reserved entity range ownership', () => {
214221

215222
expect(Array.from(engine.getEntitiesWith(Owned))).toHaveLength(0)
216223
})
224+
225+
// Drives an INBOUND message deliberately. `onChangeFunction` is only invoked from the
226+
// CRDT receive path, so a bare update() with no transport never reaches it — omitting
227+
// that required Engine option produced an engine that threw here while every other test
228+
// in this block stayed green.
229+
it('should produce a usable engine that processes an inbound message', async () => {
230+
const transport = { name: 'test', send: async () => {}, filter: () => true } as never as Parameters<
231+
typeof engine.addTransport
232+
>[0]
233+
engine.addTransport(transport)
234+
const buffer = new ReadWriteByteBuffer()
235+
const payload = new ReadWriteByteBuffer()
236+
Owned.schema.serialize({ value: 7 }, payload)
237+
PutComponentOperation.write(EntityUtils.toEntityId(600, 0), 1, Owned.componentId, payload.toBinary(), buffer)
238+
;(transport as unknown as { onmessage: (b: Uint8Array) => void }).onmessage(buffer.toBinary())
239+
240+
await engine.update(1 / 30)
241+
242+
expect(Owned.getOrNull(EntityUtils.toEntityId(600, 0))).not.toBeNull()
243+
})
217244
})
218245

219246
describe('and the bound is HIGHER than the default, so the id is NOT released', () => {
220247
let rendererOwned: Entity
221248

222249
beforeEach(() => {
223-
engine = Engine({ entityContainer: createEntityContainer({ reservedStaticEntities: 1024 }) })
250+
engine = Engine({
251+
onChangeFunction: () => {},
252+
entityContainer: createEntityContainer({ reservedStaticEntities: 1024 })
253+
})
224254
Owned = engine.defineComponent('test::owned', { value: Schemas.Int })
225255
// Number 700 is renderer-owned under a bound of 1024, but outside the default range.
226256
rendererOwned = EntityUtils.toEntityId(700, 0)

0 commit comments

Comments
 (0)