test(lib-ol): add feature-property and refresh benches - #280
Draft
pjeweb wants to merge 1 commit into
Draft
Conversation
🦋 Changeset detectedLatest commit: 4b1251c The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| function isGeometry(value: unknown): value is Geometry { | ||
| return ( | ||
| typeof value === "object" && | ||
| value !== null && |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved correctness and benchmark-fidelity issues remain, including clone ID preservation and stale backing state.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds exploratory lib-ol benchmarks for feature-property storage and OpenLayers feature refresh behavior, including an experimental LearnableFeature.
Changes:
- Adds property and refresh benchmark scripts.
- Adds dataset loading, benchmarks, and
LearnableFeaturetests. - Adds an empty changeset.
File summaries
| File | Summary |
|---|---|
packages/lib-ol/package.json |
Adds benchmark commands. |
packages/lib-ol/__tests__/feature-refresh.bench.ts |
Benchmarks refresh strategies. |
packages/lib-ol/__tests__/feature-properties/learnable-feature.ts |
Implements the feature-property prototype. |
packages/lib-ol/__tests__/feature-properties/learnable-feature.test.ts |
Tests feature behavior. |
packages/lib-ol/__tests__/feature-properties/datasets.ts |
Loads and caches GeoJSON datasets. |
packages/lib-ol/__tests__/feature-properties.bench.ts |
Benchmarks property strategies. |
.changeset/lib-ol-feature-benches.md |
Records no package version bump. |
Review details
Suppressed comments (5)
packages/lib-ol/tests/feature-properties.bench.ts:137
- This copies every property bag before the ingest strategy is selected. The
learnable-*branches then pass that copy tosetPropertiesas their backing, so the benchmark never exercises the documented by-reference backing path and its heap result includes a full shallow-copy allocation. Pass the raw property bags through for learnable strategies and only copy for the copy-based strategies.
const properties = {...source.getProperties()};
packages/lib-ol/tests/feature-properties/datasets.ts:118
- The response is parsed once to validate it and immediately parsed again to produce the returned collection. Retaining the first parsed value avoids an unnecessary full-data parse on every cache refresh, which is especially costly for the large live datasets used by these benches.
const text = await response.text();
JSON.parse(text);
await writeFile(cachePath, text);
return JSON.parse(text) as GeoJsonCollection;
packages/lib-ol/tests/feature-properties/learnable-feature.ts:122
- Using
local !== undefinedtreats an own local property whose value was explicitly set toundefinedas absent. A normalfeature.set(key, undefined)therefore falls through tobacking[key]and cannot clear or mask the backing value; check whether the local bag owns the key instead of checking its value.
if (local !== undefined || key === this.getGeometryName()) {
packages/lib-ol/tests/feature-properties/learnable-feature.ts:56
- The only miss test supplies a custom handler, which is intentionally called twice, so it does not exercise
defaultOnMiss's once-per-key warning behavior or the reset helper. Add a case using the default handler and spy onconsole.warnto verify two reads emit one warning.
function defaultOnMiss(key: string): void {
if (warnedMissKeys.has(key)) {
return;
}
packages/lib-ol/tests/feature-refresh.bench.ts:153
- When an existing feature has no id, this removes it but leaves
hasChangedfalse. If that is the only change, the finalsource.changed()is skipped, unlikecore.updateFeaturesInSource, so the benchmark's source-notification behavior is wrong for valid id-less GeoJSON features.
} else {
source.removeFeature(feature);
}
- Files reviewed: 7/7 changed files
- Comments generated: 9
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+168
to
+169
| }); | ||
| clone.setGeometryName(this.getGeometryName()); |
|
|
||
| expect(clone).toBeInstanceOf(LearnableFeature); | ||
| expect(clone.get("name")).toBe("A"); | ||
| expect(clone.getBacking()).toEqual(backing); |
Comment on lines
+111
to
+114
| setBacking(backing: Record<string, unknown> | null): void { | ||
| this.backing = backing; | ||
| this.applyCoreFromBacking(true); | ||
| } |
Comment on lines
+132
to
+135
| if (featureChanged) { | ||
| counters.changedFeatures += 1; | ||
| baseFeature.changed(); | ||
| } |
Comment on lines
+214
to
+216
| /** Force a distinct string object so === measures a real O(n) compare. */ | ||
| function detachString(text: string): string { | ||
| return (" " + text).slice(1); |
Comment on lines
+32
to
+36
| it("serves backing keys from get() and warns once per key", () => { | ||
| resetLearnableFeatureMissWarningsForTests(); | ||
| const onMiss = vi.fn(); | ||
| const feature = new LearnableFeature(new Point([1, 2]), { | ||
| backing: {description: "html", name: "Stop"}, |
Comment on lines
+10
to
+14
| export const DEFAULT_CORE_PROPERTY_KEYS = [ | ||
| "id", | ||
| "name", | ||
| "title", | ||
| "type", |
Comment on lines
+10
to
+14
| * - baseline core behavior today; geometry is diffed by reference, | ||
| * and readFeatures always creates new Geometry objects | ||
| * - geometry-aware skip the geometry set when flat coordinates are equal | ||
| * - core-keys geometry-aware + diff only core/style keys | ||
| * - text-skip compare the raw response text and skip everything |
Comment on lines
+44
to
+48
| function geometriesEqual(left: unknown, right: unknown): boolean { | ||
| if (left === right) { | ||
| return true; | ||
| } | ||
|
|
pjeweb
marked this pull request as draft
September 11, 2026 12:39
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
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
bench:propertiesandbench:refreshpackage scripts.Test plan
pnpm --filter lib-ol exec vitest run __tests__/feature-properties/learnable-feature.test.tspnpm changeset status --since=origin/mainreports no packages to bump