Ma77Ball commented on code in PR #6691:
URL: https://github.com/apache/texera/pull/6691#discussion_r3627120513
##########
frontend/src/app/workspace/service/preset/preset.service.spec.ts:
##########
@@ -456,4 +456,122 @@ describe("PresetService", () => {
});
});
});
+
+ describe("updateOrCreatePreset", () => {
+ // fetchKey is backed by a synchronous `of(...)`, so the subscribe body
(and
+ // the savePresets write-through it triggers) runs before the call returns.
+ it("is a no-op when the original and replacement presets are identical",
() => {
Review Comment:
Renamed the test to describe what actually holds: the stored preset list is
written back unchanged, rather than implying no side effect. Thanks for
catching the misleading name.
##########
frontend/src/app/workspace/service/preset/preset.service.spec.ts:
##########
@@ -456,4 +456,122 @@ describe("PresetService", () => {
});
});
});
+
+ describe("updateOrCreatePreset", () => {
+ // fetchKey is backed by a synchronous `of(...)`, so the subscribe body
(and
+ // the savePresets write-through it triggers) runs before the call returns.
+ it("is a no-op when the original and replacement presets are identical",
() => {
+ const stored: Preset[] = [{ presetProperty: "v1" }];
+ userConfigStub.fetchKey.mockReturnValue(of(JSON.stringify(stored)));
+
+ presetService.updateOrCreatePreset(presetType, presetTarget, {
presetProperty: "x" }, { presetProperty: "x" });
+
+ // list is written back unchanged: neither pushed, replaced, nor spliced.
+ expect(userConfigStub.set).toHaveBeenCalledWith(presetDictKey,
JSON.stringify(stored));
+ });
+
+ it("appends the replacement when neither preset already exists", () => {
+ userConfigStub.fetchKey.mockReturnValue(of(JSON.stringify([{
presetProperty: "v1" }])));
+
+ presetService.updateOrCreatePreset(
+ presetType,
+ presetTarget,
+ { presetProperty: "missing" },
+ { presetProperty: "v2" }
+ );
+
+ expect(userConfigStub.set).toHaveBeenCalledWith(
+ presetDictKey,
+ JSON.stringify([{ presetProperty: "v1" }, { presetProperty: "v2" }])
+ );
+ });
+
+ it("is a no-op when only the replacement preset already exists", () => {
Review Comment:
Same fix here: renamed the test to say the stored preset list is written
back unchanged, since `savePresets` still writes even when the list is not
mutated.
##########
frontend/src/app/workspace/service/preset/preset.service.spec.ts:
##########
@@ -456,4 +456,122 @@ describe("PresetService", () => {
});
});
});
+
+ describe("updateOrCreatePreset", () => {
+ // fetchKey is backed by a synchronous `of(...)`, so the subscribe body
(and
+ // the savePresets write-through it triggers) runs before the call returns.
+ it("is a no-op when the original and replacement presets are identical",
() => {
+ const stored: Preset[] = [{ presetProperty: "v1" }];
+ userConfigStub.fetchKey.mockReturnValue(of(JSON.stringify(stored)));
+
+ presetService.updateOrCreatePreset(presetType, presetTarget, {
presetProperty: "x" }, { presetProperty: "x" });
+
+ // list is written back unchanged: neither pushed, replaced, nor spliced.
+ expect(userConfigStub.set).toHaveBeenCalledWith(presetDictKey,
JSON.stringify(stored));
+ });
+
+ it("appends the replacement when neither preset already exists", () => {
+ userConfigStub.fetchKey.mockReturnValue(of(JSON.stringify([{
presetProperty: "v1" }])));
+
+ presetService.updateOrCreatePreset(
+ presetType,
+ presetTarget,
+ { presetProperty: "missing" },
+ { presetProperty: "v2" }
+ );
+
+ expect(userConfigStub.set).toHaveBeenCalledWith(
+ presetDictKey,
+ JSON.stringify([{ presetProperty: "v1" }, { presetProperty: "v2" }])
+ );
+ });
+
+ it("is a no-op when only the replacement preset already exists", () => {
+ const stored: Preset[] = [{ presetProperty: "v1" }, { presetProperty:
"v2" }];
+ userConfigStub.fetchKey.mockReturnValue(of(JSON.stringify(stored)));
+
+ presetService.updateOrCreatePreset(
+ presetType,
+ presetTarget,
+ { presetProperty: "missing" },
+ { presetProperty: "v2" }
+ );
+
+ expect(userConfigStub.set).toHaveBeenCalledWith(presetDictKey,
JSON.stringify(stored));
+ });
+
+ it("implicitly deletes a preset when both the original and the replacement
exist", () => {
+ userConfigStub.fetchKey.mockReturnValue(of(JSON.stringify([{
presetProperty: "v1" }, { presetProperty: "v2" }])));
+
+ // Both presets are present (membership is checked deeply via isEqual),
so the
+ // implicit-delete branch runs. Note: the branch splices at lodash
indexOf, which
+ // uses reference equality; the presets are freshly JSON-parsed objects,
so
+ // indexOf returns -1 and splice(-1, 1) drops the last element (v2).
+ presetService.updateOrCreatePreset(presetType, presetTarget, {
presetProperty: "v1" }, { presetProperty: "v2" });
+
+ expect(userConfigStub.set).toHaveBeenCalledWith(presetDictKey,
JSON.stringify([{ presetProperty: "v1" }]));
+ });
Review Comment:
Confirmed the bug: membership is checked with deep `isEqual` but the delete
branch used reference-based lodash `indexOf`, so on JSON-parsed presets it
returned -1 and spliced the wrong element. Switched to a deep-equality index
(`findIndex` + `isEqual`) and updated the test to assert the original is
removed, leaving the replacement.
##########
frontend/src/app/workspace/service/preset/preset.service.spec.ts:
##########
@@ -456,4 +456,122 @@ describe("PresetService", () => {
});
});
});
+
+ describe("updateOrCreatePreset", () => {
+ // fetchKey is backed by a synchronous `of(...)`, so the subscribe body
(and
+ // the savePresets write-through it triggers) runs before the call returns.
+ it("is a no-op when the original and replacement presets are identical",
() => {
+ const stored: Preset[] = [{ presetProperty: "v1" }];
+ userConfigStub.fetchKey.mockReturnValue(of(JSON.stringify(stored)));
+
+ presetService.updateOrCreatePreset(presetType, presetTarget, {
presetProperty: "x" }, { presetProperty: "x" });
+
+ // list is written back unchanged: neither pushed, replaced, nor spliced.
+ expect(userConfigStub.set).toHaveBeenCalledWith(presetDictKey,
JSON.stringify(stored));
+ });
+
+ it("appends the replacement when neither preset already exists", () => {
+ userConfigStub.fetchKey.mockReturnValue(of(JSON.stringify([{
presetProperty: "v1" }])));
+
+ presetService.updateOrCreatePreset(
+ presetType,
+ presetTarget,
+ { presetProperty: "missing" },
+ { presetProperty: "v2" }
+ );
+
+ expect(userConfigStub.set).toHaveBeenCalledWith(
+ presetDictKey,
+ JSON.stringify([{ presetProperty: "v1" }, { presetProperty: "v2" }])
+ );
+ });
+
+ it("is a no-op when only the replacement preset already exists", () => {
+ const stored: Preset[] = [{ presetProperty: "v1" }, { presetProperty:
"v2" }];
+ userConfigStub.fetchKey.mockReturnValue(of(JSON.stringify(stored)));
+
+ presetService.updateOrCreatePreset(
+ presetType,
+ presetTarget,
+ { presetProperty: "missing" },
+ { presetProperty: "v2" }
+ );
+
+ expect(userConfigStub.set).toHaveBeenCalledWith(presetDictKey,
JSON.stringify(stored));
+ });
+
+ it("implicitly deletes a preset when both the original and the replacement
exist", () => {
+ userConfigStub.fetchKey.mockReturnValue(of(JSON.stringify([{
presetProperty: "v1" }, { presetProperty: "v2" }])));
+
+ // Both presets are present (membership is checked deeply via isEqual),
so the
+ // implicit-delete branch runs. Note: the branch splices at lodash
indexOf, which
+ // uses reference equality; the presets are freshly JSON-parsed objects,
so
+ // indexOf returns -1 and splice(-1, 1) drops the last element (v2).
+ presetService.updateOrCreatePreset(presetType, presetTarget, {
presetProperty: "v1" }, { presetProperty: "v2" });
+
+ expect(userConfigStub.set).toHaveBeenCalledWith(presetDictKey,
JSON.stringify([{ presetProperty: "v1" }]));
+ });
+
+ it("takes the in-place replace branch when only the original exists", ()
=> {
+ userConfigStub.fetchKey.mockReturnValue(of(JSON.stringify([{
presetProperty: "v1" }, { presetProperty: "v2" }])));
+
+ // The original exists (deep match) but the replacement does not, so the
else
+ // (replace) branch runs: presets[indexOf(presets, original)] =
replacement.
+ // indexOf is reference-based and returns -1, so this assigns the "-1"
key rather
+ // than an array index; JSON.stringify ignores it and the array
serializes unchanged.
+ presetService.updateOrCreatePreset(presetType, presetTarget, {
presetProperty: "v1" }, { presetProperty: "v3" });
+
+ expect(userConfigStub.set).toHaveBeenCalledWith(
+ presetDictKey,
+ JSON.stringify([{ presetProperty: "v1" }, { presetProperty: "v2" }])
+ );
+ });
Review Comment:
Same root cause fixed here: the replace branch also used reference
`indexOf`, so it wrote to index -1 and the array serialized unchanged. Now it
replaces the original in place via a deep-equality index, and the test asserts
that behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
