search

NYCPlanning / ae-zoning-api

This application is API for serving data related to zoning and tax lots.
2 stars 0 forks source link

155/get zoning districts by tax lot bbl tests #173

Closed pratishta closed 7 months ago

pratishta commented 7 months ago

Closes #155

Mocking tax lot and zoning districts relationship

I opted to use an object with the bbl string as a computed property name that relates to the array of zoning districts [ { '1588702147': [ [Object], [Object], [Object] ] } ]

It looks similar to the tuple suggestion from @TangoYankee [ [ { bbl: '1588702147' }, [ [Object], [Object], [Object] ] ] ]

But I think that generating a computed key with the bbl string takes care of the extra overhead while still being intuitive to interpret as a relation.

Seeding

I updated the tax lot mocks so that we are seeding once in checkTaxLotByBblMocks and having other mock functions get their bbls from there. I also changed all arrays to only have one tax lot since we don't need multiple (as of now).

I will squash the last two commits to 1 seed change commit once approved.

pratishta commented 7 months ago

Updated this PR with a few changes and options that are specific to commits (I'll squash appropriately when we've decided how to move forward)

Option 1 (6158eba6f89bb8f97ec5ad055e702e60a535b5fb) - Using checkTaxLotByBbl mocks as the initial array generation that informs the remaining mocks' bbls via the spread operator (thank you @TangoYankee for the idea in this draft PR). Since the spread operator merges the initially created bbls, we don't need to seed again

Option 2 (52782bd451762f03783f1c2fc8bb671bef3fd3f0) - Not using checkTaxLotByBbl mocks as the initial array generation and generating mocks separately instead (suggested here). The { seed: seed + 1 } here keeps the bbls consistent across mocks. For some reason, seeding with 0 produces inconsistent results and I haven't spent enough time looking into Faker's or zod-mock's source code to determine exactly why.

Option 3 (382353f770c092462e3a76dbe0d5b299ae41ff94) - Not using checkTaxLotByBbl mocks as the initial array generation and generating mocks separately instead, and just letting them be inconsistent across unrelated mocks. Since we're ultimately testing against the schema, it's fine if the bbls are not the same.

I think Option 1 or Option 3 are fine, but would love some feedback (including whether the changes to mocking should be in a separate PR entirely).

TangoYankee commented 7 months ago

Updated this PR with a few changes and options that are specific to commits (I'll squash appropriately when we've decided how to move forward)

Option 1 (6158eba) - Using checkTaxLotByBbl mocks as the initial array generation that informs the remaining mocks' bbls via the spread operator (thank you @TangoYankee for the idea in this draft PR). Since the spread operator merges the initially created bbls, we don't need to seed again

Option 2 (52782bd) - Not using checkTaxLotByBbl mocks as the initial array generation and generating mocks separately instead (suggested here). The { seed: seed + 1 } here keeps the bbls consistent across mocks. For some reason, seeding with 0 produces inconsistent results and I haven't spent enough time looking into Faker's or zod-mock's source code to determine exactly why.

Option 3 (382353f) - Not using checkTaxLotByBbl mocks as the initial array generation and generating mocks separately instead, and just letting them be inconsistent across unrelated mocks. Since we're ultimately testing against the schema, it's fine if the bbls are not the same.

I think Option 1 or Option 3 are fine, but would love some feedback (including whether the changes to mocking should be in a separate PR entirely).

Let's go with option 2. I didn't realize that 0 seeds were inconsistent. We'll need consistency to make sure when we check for "missing" ids that they're truly missing and no randomness accidentally caused a match. Good find.