Repository navigation
test(lint): cover template-root composition variable declarations - #6150
Conversation
miguel-heygen
left a comment
There was a problem hiding this comment.
Thanks for tracking this one down and closing the loop. I confirmed the claim against main: the rule validates every element carrying the attribute, not just the root element, so the defect is already fixed and a test is the right remaining piece. The new case takes the same code path as the existing root element test, so it is close to redundant, but it pins the exact reproduction from the issue and that is worth keeping.
CI has not run on this head yet, only the mergeability and WIP checks, so a maintainer will need to approve the workflow run before this merges.
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at 004afd60, adding to Miguel's approval at the same head.
I checked the premise: #4454 (571c99ce9, on main) made the rule validate every element that carries data-composition-variables, so the bug in #4383 is already fixed and a test is the right remaining piece. I applied this test onto current main and the composition suite passes 202 of 202. Like Miguel said, it takes the same path as the existing root-element case, but it pins the exact reproduction from the issue, which is worth having before Closes #4383 fires.
One thing will block the merge: GitHub reports the commit as unsigned, and main requires signed commits. Re-signing it (for example git commit --amend --no-edit -S and a force push) and adding the key to GitHub as a signing key will clear that.
Need: yes, it closes the loop on #4383 · Size: ok · Reuse: extends the existing describe block · Simpler: already minimal
— Rames
|
Thanks, @ym256-stanford. This one is approved, and it'll go into the merge queue once these are sorted:
Once it's green and signed, we'll queue it at your new head. If you push code changes beyond a rebase, we'll take another look first. — Rames |
|
This is approved, but main requires signed commits. Could you re-sign ( |
The declaration rule validates data-composition-variables on every declaring element since 571c99c (heygen-com#4454), but the template-fragment reproduction case from heygen-com#4383 had no regression coverage. Add it. Refs heygen-com#4383 Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Head branch was pushed to by a user without write access
004afd6 to
5d45d24
Compare
|
@jrusso1020 Thanks for the review! I’ve re-signed and pushed the commit, and GitHub now shows it as verified. The PR remains approved and has no merge conflicts, but the workflows are awaiting maintainer approval. Could you approve the CI runs and, once the required checks pass, add it to the merge queue? Happy to address anything CI finds. |
Port the adopted upstream fixes from f5bde95 to the current upstream tip: heygen-com#7375 multipart cloud upload, heygen-com#7382 minParallelChunks, heygen-com#7349 HLG header SEI, heygen-com#7369 skip unmounted compositions in lint/check, heygen-com#7381 filesystem-path media in the cloud archive, heygen-com#7383 Safari cut-point pre-roll revert, heygen-com#7373 clock steering between audio time updates, heygen-com#7384 injector test isolation, heygen-com#7378 preview seek retention, heygen-com#7390 virtual timer schedule per worker, heygen-com#7394 CDN script failure naming, heygen-com#7395 menu row radius, heygen-com#7119 decode timeout caller test, heygen-com#7397 authored tween order, heygen-com#7402 capture page clip loading, heygen-com#7412 muted video audio track at cuts, heygen-com#6150 template root variable test, heygen-com#7442 held tail seek once. Deliberate local differences: heygen-com#7067's audio-start hold is not carried into clock.ts, mountedSources comes from heygen-com#6827 as a helper only, cloud/_gen/types.ts omits the image-request types the fork does not generate, and the lint symlink test skips through symlinkOrSkip where the account cannot create links. Co-Authored-By: Claude Sonnet <noreply@anthropic.com>
Summary
Adds regression coverage for the
<template>-fragment reproduction case from #4383. The defect itself —invalid_composition_variables_declarationonly validatingdata-composition-variableson<html>— was already fixed by #4454 (571c99c), which now validates every element carrying the attribute. The fix landed without referencing the issue, so it stayed open; this PR closes the loop with the missing test.Evidence
Ran the issue's reproduction script verbatim against
main(deeb8d4):All three malformed declarations are now diagnosed, matching the issue's expected output. The existing root case was covered by a test added in #4454; the template-fragment case (
<template>+isSubComposition: true) was not — this PR adds it.Validation
bun run --cwd packages/lint test src/rules/composition.test.ts— 202/202 passbunx oxlint/bunx oxfmt --checkon the changed file — cleanCloses #4383