Repository navigation
Add WPT mimesniff/mime-types test - #8131
Conversation
![clanky-the-clanker[bot]](https://avatars.githubusercontent.com/in/5306670?s=60&v=4){kind=small}
There was a problem hiding this comment.
Superseded by the review of
dd3c6ab2.
This review, of 4b4e8791
💬 Approved with comments — The change only adds a test and its build target. One INFO note: a comment's summary of Blob type normalization is slightly inaccurate.
Findings: 1 info
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📝 Documentation: 💬 1 info
One test comment slightly misstates Blob type normalization
Recommendation: 💬 Optional Nits
- [INFO] The comment says workerd lowercases printable-ASCII types and discards others, but
normalizeTypealso keeps U+007F, which is not printable (line 24 mentions this). Consider noting the exception in the summary. (src/wpt/mimesniff/mime-types-test.ts:17)
View full review
src/wpt/mimesniff/mime-types-test.ts:16-17says workerd only lowercases printable-ASCII types and discards others.normalizeTypealso keeps U+007F, which is not printable. The comment at line 24 already mentions this exception, so the summary could note it too.
✅ All other checks passed
🔒 Security: ✅ LGTM
No security concerns; the change only adds a test.
📖 AGENTS.md: ✅ LGTM
No AGENTS.md concerns.
📋 Code Quality: ✅ LGTM
The new wpt_test target follows the same pattern as the targets next to it. expectedFailures mixes strings and regexes, which the harness type (string | RegExp)[] accepts. The source comments match normalizeType in src/workerd/api/blob.c++. Nothing here changes production behaviour.
⚡ Performance: ✅ LGTM
No performance concerns.
Reviewed commit: 4b4e8791
4b4e879 to
dd3c6ab
Compare
There was a problem hiding this comment.
Superseded by the review of
9d28f724.
This review, of dd3c6ab2
💬 Approved with comments — The change only adds a test and its build target, with no production impact. One INFO note remains on a comment's wording.
Findings: 1 info
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📝 Documentation: 💬 1 info
The Blob/File summary comment leaves out the U+007F exception.
Recommendation: 💬 Optional Nits
- [INFO] The comment says workerd lowercases printable-ASCII types and discards the others, but
normalizeTypealso keeps U+007F, which is not printable (line 24 mentions this). Consider noting the exception in the summary. (src/wpt/mimesniff/mime-types-test.ts:17)
View full review
src/wpt/mimesniff/mime-types-test.ts:16-17 says workerd only lowercases printable-ASCII types and discards the others. normalizeType also keeps U+007F, which is not printable. The comment at line 24 already mentions this exception, so the summary could mention it too.
✅ All other checks passed
🔒 Security: ✅ LGTM
No security concerns. The change only adds a test config and a build target.
📖 AGENTS.md: ✅ LGTM
No AGENTS.md concerns.
📋 Code Quality: ✅ LGTM
The mimesniff/mime-types target in src/wpt/BUILD.bazel follows the same pattern as the wpt_test targets next to it. The new config file falls under the **/*.ts include in src/wpt/tsconfig.json.
⚡ Performance: ✅ LGTM
No performance concerns. The change only adds test configuration.
Reviewed commit: dd3c6ab2
parsing.any.js runs the WHATWG MIME Sniffing parser vectors
(mime-types.json and generated-mime-types.json) through two paths:
new Blob/File({type}).type, and the type of a Blob read from a
Request or Response with that Content-Type header.
1285 of 1898 subtests pass. Request/Response passes 933 of 942.
Its failures are workerd MIME parser bugs (backslash treated as a
token code point; an invalid parameter name does not consume its
quoted value), Fetch's comma splitting in "extract a MIME type",
and workerd accepting non-Latin-1 header values. The other 604
failures are Blob/File cases: the test expects MIME parsing, but
the File API spec only lowercases the type, as workerd does.
Browsers fail these heavily too.
charset-parameter.window.js is omitted because it needs iframes.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dd3c6ab to
9d28f72
Compare
There was a problem hiding this comment.
✅ Approved — The pull request only adds a WPT test config and its build target. No new issues; one earlier wording note is still open and left to the author.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
No security concerns. The change only adds a test config and a build target.
📖 AGENTS.md: ✅ LGTM
No AGENTS.md concerns.
📋 Code Quality: ✅ LGTM
The new mimesniff/mime-types wpt_test target in src/wpt/BUILD.bazel matches the targets next to it, such as dom/abort and dom/events. It uses a nested name, a config path under src/wpt/, and a @wpt//:<dir>@module directory label.
⚡ Performance: ✅ LGTM
No performance concerns. The change only adds test configuration.
📝 Documentation: ✅ LGTM
No new documentation issues. One earlier wording note on src/wpt/mimesniff/mime-types-test.ts is still open and left to the author.
Reviewed commit: 9d28f724
Adds a WPT target for
mimesniff/mime-types: 1285/1898 subtests pass.\accepted as a token code point; an invalid parameter name doesn't consume its quoted value), Fetch's comma splitting, and workerd accepting non-Latin-1 header values.type, as workerd does. Browsers fail these too.🤖 Generated with Claude Code