Skip to content

test: cover null chart style validation - #314

Open
ahfoysal wants to merge 3 commits into
antvis:mainfrom
ahfoysal:test-reject-null-chart-style
Open

test: cover null chart style validation#314
ahfoysal wants to merge 3 commits into
antvis:mainfrom
ahfoysal:test-reject-null-chart-style

Conversation

@ahfoysal

@ahfoysal ahfoysal commented Jul 5, 2026

Copy link
Copy Markdown

Summary

  • add a regression test for style: null chart input so it is rejected as invalid params before chart generation
  • clean up the area chart tool description grammar
  • keep the check local to callTool so the test does not call the remote chart service

Context

Issue #313 shows style: null reaching the renderer and failing with Cannot read properties of null. Current main already rejects that input at validation time; this PR locks that behavior down so it does not regress.

Verification

  • npm install
  • npm run build
  • npm test -- --run __tests__/utils/callTool.spec.ts __tests__/charts/charts.spec.ts
  • npm test -- --run
  • npx biome check __tests__/utils/callTool.spec.ts src/charts/area.ts __tests__/charts/area.json
  • git diff --check

Refs #313.

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

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.

Code Review

This pull request fixes a grammatical typo in the area chart description and adds a new unit test to verify that callTool rejects a null style parameter. The reviewer suggested simplifying the test case by removing optional parameters that are not relevant to the specific validation being tested.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread __tests__/utils/callTool.spec.ts Outdated
@ahfoysal

ahfoysal commented Jul 5, 2026

Copy link
Copy Markdown
Author

The new test only checks for a generic -32602 error, so it does not actually prove that chart generation is skipped before any rendering work starts. If callTool() still invokes downstream generation and the renderer later throws the same error code, this test would pass anyway. I would either spy on the generation path or assert on the specific validation message/source so the regression stays pinned to the null-style guard.

@hustcc

hustcc commented Jul 5, 2026

Copy link
Copy Markdown
Member

can help to fix the issue?

@ahfoysal
ahfoysal force-pushed the test-reject-null-chart-style branch from 17d4e1e to da0c916 Compare July 5, 2026 16:19
@ahfoysal

ahfoysal commented Jul 5, 2026

Copy link
Copy Markdown
Author

Fixed in the latest commit. I updated the test to check the specific null style validation message and added a spy to make sure generateChartUrl is not called when validation fails.

Checked locally:

  • npm test -- __tests__/utils/callTool.spec.ts
  • npx biome check __tests__/utils/callTool.spec.ts
  • npm run build

@ahfoysal

ahfoysal commented Jul 6, 2026

Copy link
Copy Markdown
Author

Verified on branch with latest commit:

  • npm run build
  • npm test -- --run tests/utils/callTool.spec.ts tests/charts/charts.spec.ts
  • npx biome check tests/utils/callTool.spec.ts src/charts/area.ts tests/charts/area.json

callTool now rejects style:null with the explicit validation message, and the generateChartUrl path is not reached for that case.

@hustcc

hustcc commented Jul 6, 2026

Copy link
Copy Markdown
Member

this is not fix the issue, just add an test case. Process the style = null case, make it works, this is fix the issue.

@ahfoysal

ahfoysal commented Jul 6, 2026

Copy link
Copy Markdown
Author

Updated. You were right — the previous change only tested the rejection path.

This now handles style: null by treating it the same as an omitted style, so chart generation continues normally. The test was updated to assert that generateChartUrl is called with style removed from the payload.

Checked locally:

  • npm test -- --run __tests__/utils/callTool.spec.ts __tests__/charts/charts.spec.ts
  • npx biome check src/utils/callTool.ts __tests__/utils/callTool.spec.ts
  • npm run build

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants