Copilot commented on code in PR #6691:
URL: https://github.com/apache/texera/pull/6691#discussion_r3626779600
##########
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:
The test name says this is a "no-op", but the implementation still calls
savePresets, which writes through to UserConfigService.set. That makes the
behavior not a true no-op (it has observable side effects). Consider renaming
the test to describe what actually stays unchanged (the preset list), rather
than implying no write happens.
##########
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 issue as above: this test is labeled "no-op" but it still expects a
write-back via UserConfigService.set. Rename it to reflect that the stored list
is unchanged, not that the method performs no side effects.
##########
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:
This test is currently locking in another likely-buggy behavior from
updateOrCreatePreset: when only the original exists, the method should replace
it with the replacement. Because lodash indexOf is reference-based, the current
implementation writes to presets[-1] and the serialized array stays unchanged;
the test expects that unchanged array. It would be better for this test to
assert the intended replacement behavior and update the production code to use
a deep-equality index (findIndex/isEqual).
##########
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:
This test asserts behavior that appears to be an implementation bug in
updateOrCreatePreset: the code checks membership via deep equality (contains
uses isEqual), but deletes via lodash indexOf (reference equality). With
freshly JSON-parsed objects, indexOf returns -1 and splice(-1, 1) removes the
last preset instead of the original. The expected result should be deleting the
original preset (leaving the replacement), and the production code should use a
deep-equality index (e.g., findIndex/isEqual) rather than lodash indexOf.
--
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]
