Skip to content

fix: align v0.9 slider schema with spec#1487

Open
kiwigitops wants to merge 2 commits into
google:mainfrom
kiwigitops:fix-v09-slider-step-schema
Open

fix: align v0.9 slider schema with spec#1487
kiwigitops wants to merge 2 commits into
google:mainfrom
kiwigitops:fix-v09-slider-step-schema

Conversation

@kiwigitops
Copy link
Copy Markdown

Closes #1384.

Summary

  • remove the non-spec step property from the v0.9 WebCore SliderApi schema
  • stop the Angular v0.9 slider from reading that non-spec prop
  • add a v0.9 schema regression so Slider.step is rejected instead of diverging from the frozen JSON catalog

Verification

  • npm test -- --watch=false from renderers/web_core
  • npx ng build lib from renderers/angular

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request removes the step property from the SliderApi schema and the corresponding Angular SliderComponent. A regression test was added to verify that the schema now rejects the step property. Feedback suggests improving the robustness of this test by ensuring it specifically fails due to the unrecognized step key rather than other potential validation errors, and provided a code suggestion to implement this check.

Comment on lines +51 to +59
it('should reject non-spec step property', () => {
const slider = {
max: 100,
step: 5,
value: {literalNumber: 50},
};

assert.throws(() => SliderApi.schema.parse(slider));
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The regression test for SliderApi should be more robust. Currently, assert.throws will pass if the schema parsing fails for any reason (e.g., an invalid value format or missing required fields). To ensure it specifically catches the rejection of the step property, it is better to first validate a correct object and then verify that adding step causes a specific error. Additionally, using a literal number for value is more consistent with other tests in this file.

    it('should reject non-spec step property', () => {
      const validSlider = {
        max: 100,
        value: 50,
      };
      // Verify it parses correctly without the 'step' property
      SliderApi.schema.parse(validSlider);

      const invalidSlider = {
        ...validSlider,
        step: 5,
      };

      assert.throws(
        () => SliderApi.schema.parse(invalidSlider),
        /unrecognized_keys.*step/i
      );
    });

@kiwigitops
Copy link
Copy Markdown
Author

Tightened the regression in 192683b so it first verifies the slider parses without step, then checks that adding step is rejected as the unrecognized key. npm test -- --watch=false still passes in renderers/web_core.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

Slider.step is in WebCore Zod but missing from the JSON Schema spec

1 participant