Add UploadButton unit tests#13336
Conversation
🪼 branch checks and previews
Install Gradio from this PR pip install https://huggingface.co/buckets/gradio/pypi-previews/resolve/4fb675ec39aad9a238de1d41d386ead21933fcb8/gradio-6.13.0-py3-none-any.whlInstall Gradio Python Client from this PR pip install "gradio-client @ git+https://github.com/gradio-app/gradio@4fb675ec39aad9a238de1d41d386ead21933fcb8#subdirectory=client/python"Install Gradio JS Client from this PR npm install https://gradio-npm-previews.s3.amazonaws.com/4fb675ec39aad9a238de1d41d386ead21933fcb8/gradio-client-2.2.0.tgz |
🦄 change detectedThis Pull Request includes changes to the following packages.
✅ Changeset approved by @freddyaboulton
|
freddyaboulton
requested review from
dawoodkhan82,
hannahblair,
hysts and
pngwn
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
pngwn
left a comment
pngwn
left a comment
There was a problem hiding this comment.
Reviewed against the frontend unit testing skill. Solid foundation — run_shared_prop_tests invocation, mount-time event handling with { retrospective: true }, semantic queries, and test.todo placeholders for visual-only props are all on point.
Main feedback (inline):
Props: file_counttests assert HTML attributes instead of behaviour. The skill is explicit that this is the wrong shape for this kind of test — drive uploads and assert on the resulting value instead.Props: file_typeshas the same shape, plus a redundant "empty array → empty string" test and a missing null-input case.get_data / set_datablock is missing theset_datahalf and the user-interaction round-trip — both are required by the skill.- The
max_file_sizeprop has no coverage at all; it should be asserted to flow through toclient.upload. - Minor: the manual
{ upload, stream }object in the error test can be replaced with{ ...mock_client(), upload: failing_upload }.
Plus a few nits inline (icon src assertion, null as any casts, click-side-effect clarification).
Generated by Claude Code
| describe("Props: file_count", () => { | ||
| afterEach(() => cleanup()); | ||
|
|
||
| test("file_count='single' does not set multiple attribute on input", async () => { | ||
| const { getByTestId } = await render(UploadButton, { | ||
| ...base_props, | ||
| file_count: "single" | ||
| }); | ||
|
|
||
| const input = getByTestId("Upload-upload-button"); | ||
| expect(input).not.toHaveAttribute("multiple"); | ||
| }); | ||
|
|
||
| test("file_count='multiple' sets multiple attribute on input", async () => { | ||
| const { getByTestId } = await render(UploadButton, { | ||
| ...base_props, | ||
| file_count: "multiple" | ||
| }); | ||
|
|
||
| const input = getByTestId("Upload-upload-button"); | ||
| expect(input).toHaveAttribute("multiple"); | ||
| }); | ||
|
|
||
| test("file_count='directory' sets webkitdirectory attribute on input", async () => { | ||
| const { getByTestId } = await render(UploadButton, { | ||
| ...base_props, | ||
| file_count: "directory" | ||
| }); | ||
|
|
||
| const input = getByTestId("Upload-upload-button"); | ||
| expect(input).toHaveAttribute("webkitdirectory"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
These tests assert on attributes (multiple, webkitdirectory) rather than user-visible behaviour. Per the frontend testing skill: "BAD: assert that an input has a step attribute set to 5. GOOD: type a value, click the increment button, and assert the value increased by 5."
file_count has an observable behavioural effect — the number of files retained after upload — so the test should drive that:
file_count='single'+upload_fileof two files →get_data().valueis a singleFileData(not an array of length 2).file_count='multiple'+ multi-file upload →get_data().valueis an array of length N.file_count='directory'is the only one for which a behavioural test is genuinely difficult; thewebkitdirectory/mozdirectoryattribute is the user-visible mechanism. Keep an attribute assertion here, but add a one-line comment explaining why (the skill calls for a comment + rationale when going against the "test behaviour" principle).
Generated by Claude Code
| describe("Props: file_types", () => { | ||
| afterEach(() => cleanup()); | ||
|
|
||
| test("file_types maps category names to wildcard MIME types", async () => { | ||
| const { getByTestId } = await render(UploadButton, { | ||
| ...base_props, | ||
| file_types: ["image", "audio"] | ||
| }); | ||
|
|
||
| const input = getByTestId("Upload-upload-button"); | ||
| expect(input).toHaveAttribute("accept", "image/*, audio/*"); | ||
| }); | ||
|
|
||
| test("file_types passes dot-prefixed extensions through unchanged", async () => { | ||
| const { getByTestId } = await render(UploadButton, { | ||
| ...base_props, | ||
| file_types: [".csv", ".json"] | ||
| }); | ||
|
|
||
| const input = getByTestId("Upload-upload-button"); | ||
| expect(input).toHaveAttribute("accept", ".csv, .json"); | ||
| }); | ||
|
|
||
| test("empty file_types array results in empty accept attribute", async () => { | ||
| const { getByTestId } = await render(UploadButton, { | ||
| ...base_props, | ||
| file_types: [] | ||
| }); | ||
|
|
||
| const input = getByTestId("Upload-upload-button"); | ||
| expect(input).toHaveAttribute("accept", ""); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Same critique as file_count: these are attribute assertions. accept is closer to legitimate territory than multiple because it is the user-visible mechanism (it's what the OS file picker reads), but three near-identical string assertions is implementation-heavy.
Suggestions:
- Keep one attribute test (since this is the mechanism), add a brief comment justifying it, and consider merging the three sub-tests into one parameterised test.
- Add the missing
file_types == null/undefinedcase —UploadButton.svelte:55hasif (file_types == null) return null;which is currently uncovered. - The
"empty file_types array results in empty accept attribute"test asserts a quirk of the implementation (that[].join(", ")is"") rather than a meaningful user behaviour. Drop it or replace with the null-input case above.
Generated by Claude Code
There was a problem hiding this comment.
In this case I wonder if we can attempt to upload a non accepted file and then check that it doesn't upload?
Claude is being a bit confusing above.
There was a problem hiding this comment.
Ok in order to test this I had to add some logic to validate the file types in js. Before we relied on the accept attribute of the input html component and upload_file helper would overwrite it. Now the UploadButton behaves similarly to how image/audio/video components work now.
| describe("get_data / set_data", () => { | ||
| afterEach(() => cleanup()); | ||
|
|
||
| test("get_data returns null initially", async () => { | ||
| const { get_data } = await render(UploadButton, { | ||
| ...base_props, | ||
| value: null | ||
| }); | ||
|
|
||
| const data = await get_data(); | ||
| expect(data.value).toBeNull(); | ||
| }); | ||
|
|
||
| test("get_data returns initial value when mounted with one", async () => { | ||
| const { get_data } = await render(UploadButton, { | ||
| ...base_props, | ||
| value: TEST_TXT | ||
| }); | ||
|
|
||
| const data = await get_data(); | ||
| expect(data.value).toEqual(TEST_TXT); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The skill mandates four checks for the data contract: DOM reflects set_data, get_data returns current state, set_data → get_data round-trip, user interaction flows back into get_data. Only the second of those is here. Please add:
test("set_data updates the value reported by get_data", async () => {
const { set_data, get_data } = await render(UploadButton, base_props);
await set_data({ value: TEST_TXT });
expect((await get_data()).value).toEqual(TEST_TXT);
});
test("upload interaction is reflected in get_data", async () => {
const { get_data } = await render(UploadButton, upload_props);
await upload_file(TEST_TXT);
await waitFor(() =>
expect((await get_data()).value).not.toBeNull()
);
});The user-interaction round-trip is the most important one and is currently absent.
Generated by Claude Code
There was a problem hiding this comment.
Ok this involved fixing a bug in the UploadButton svelte component to get these two tests to pass. Thanks claude
| client: { | ||
| upload: failing_upload, | ||
| stream: async () => ({ onmessage: null, close: () => {} }) | ||
| } |
There was a problem hiding this comment.
Minor: mock_client() already returns this exact stream shape (see js/tootils/src/render.ts:300-305). You can tighten this to:
client: { ...mock_client(), upload: failing_upload }Avoids the duplicated stream boilerplate.
Generated by Claude Code
| test("upload and change events fire after file upload", async () => { | ||
| const { listen } = await render(UploadButton, { | ||
| ...upload_props | ||
| }); | ||
|
|
||
| const upload = listen("upload"); | ||
| const change = listen("change"); | ||
|
|
||
| await upload_file(TEST_TXT); | ||
|
|
||
| await waitFor(() => { | ||
| expect(upload).toHaveBeenCalledTimes(1); | ||
| }); | ||
| expect(change).toHaveBeenCalledTimes(1); | ||
| }); |
There was a problem hiding this comment.
Missing coverage for max_file_size. It's a meaningful prop — UploadButton.svelte:86 passes it as the fourth argument to upload(). Suggested test (sits naturally next to the upload tests):
test("max_file_size is forwarded to client.upload", async () => {
const upload_spy = vi.fn(async (file_data) => file_data);
await render(UploadButton, {
...upload_props,
max_file_size: 100,
client: { ...mock_client(), upload: upload_spy }
});
await upload_file(TEST_TXT);
await waitFor(() => expect(upload_spy).toHaveBeenCalled());
expect(upload_spy).toHaveBeenCalledWith(
expect.anything(), expect.anything(), expect.anything(), 100
);
});That's genuine behavioural coverage of the contract between the component and the client.
Generated by Claude Code
There was a problem hiding this comment.
Good catch claude
| } | ||
| }); | ||
|
|
||
| expect(getByRole("img")).toBeInTheDocument(); |
There was a problem hiding this comment.
Nit: could also assert toHaveAttribute("src", "https://example.com/icon.png") here — src on <img> is the user-visible behaviour of the icon prop, not just an implementation flag, so it's worth pinning down.
Generated by Claude Code
There was a problem hiding this comment.
Tenuous but seems reasonable. Visual test would be better.
|
I have reviewed Claude's review. It is mostly reasonable but I've commented the comments to ignore. In general looks good. |
| return type.endsWith("/*") && file_type.startsWith(category + "/"); | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Missing "file/*" wildcard causes all files to be rejected
High Severity
The new is_valid_mimetype function doesn't handle "file/*" as a special "accept all" value. The Python API documents file_types=["file"] as meaning "any file allowed," and the accept_file_types derivation transforms "file" into "file/*". Since no real MIME type starts with "file/", the wildcard match file_type.startsWith("file/") always fails, causing every file to be rejected. The existing Upload.svelte correctly handles this with an early-return check for file_accept === "file/*".
Reviewed by Cursor Bugbot for commit 8b01269. Configure here.
freddyaboulton
added
v: patch
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4fb675e. Configure here.
| return type.endsWith("/*") && file_type.startsWith(category + "/"); | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Duplicated is_valid_mimetype across upload components
Low Severity
The newly added is_valid_mimetype function in UploadButton.svelte duplicates the same function already present in js/upload/src/Upload.svelte. Both implement the same core logic (splitting accept strings, checking extensions, matching wildcard MIME categories). If a bug is found or behavior needs to change, both copies need to be updated independently. This could be extracted into a shared utility in @gradio/upload or @gradio/client.
Reviewed by Cursor Bugbot for commit 4fb675e. Configure here.
3 participants
Description
Closes: #13109
AI Disclosure
We encourage the use of AI tooling in creating PRs, but the any non-trivial use of AI needs be disclosed. E.g. if you used Claude to write a first draft, you should mention that. Trivial tab-completion doesn't need to be disclosed. You should self-review all PRs, especially if they were generated with AI.
🎯 PRs Should Target Issues
Before your create a PR, please check to see if there is an existing issue for this change. If not, please create an issue before you create this PR, unless the fix is very small.
Not adhering to this guideline will result in the PR being closed.
Testing and Formatting Your Code
PRs will only be merged if tests pass on CI. We recommend at least running the backend tests locally, please set up your Gradio environment locally and run the backed tests:
bash scripts/run_backend_tests.shPlease run these bash scripts to automatically format your code:
bash scripts/format_backend.sh, and (if you made any changes to non-Python files)bash scripts/format_frontend.shNote
Medium Risk
Behavior changes now reject non-matching
file_typesat selection time, which could impact existing upload flows relying on permissive client-side handling; otherwise changes are isolated to a single component plus tests.Overview
Adds a full Vitest suite for
UploadButtoncovering shared props,file_countreturn shapes,file_typesaccept mapping + rejection behavior, icon rendering, event dispatch semantics,get_data/set_data, andmax_file_sizeforwarding.Adjusts
Index.svelteto derivevaluedirectly fromgradio.props.value(removing the manual$state/$effectsync), and updates the shared implementation to validate selected files against the computedacceptrules (wildcard MIME categories and extensions), filtering invalid files and emitting anerrorinstead of uploading them. A changeset bumps@gradio/uploadbuttonandgradioas patch releases.Reviewed by Cursor Bugbot for commit 4fb675e. Bugbot is set up for automated code reviews on this repo. Configure here.