Strengthen UI tests with rendered-behavior assertions and add low-signal test audit#10
Strengthen UI tests with rendered-behavior assertions and add low-signal test audit#10
Conversation
📝 WalkthroughWalkthroughRefactors many tests to assert on rendered HTML/output instead of internal config mutators, reorganizes test groups (e.g., "config mutators" → "behavior"), adds textarea builder methods, and introduces a new test audit document for low-signal tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@claude full review |
|
Claude finished @bbopen's task in 2m 12s —— View job Code Review
SummaryThis PR is a solid step toward higher-signal tests. The audit document is a useful artifact, and several files — especially
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/direction_test.gleam (1)
56-59: 🧹 Nitpick | 🔵 TrivialHelper round-trip retained alongside render check.
The
use_directionassertion (Lines 56-59) is a helper round-trip that the audit document flags as low-signal. However, since it's paired with a subsequent render check, it provides some value as a sanity check. Consider whether this intermediate assertion adds meaningful coverage or could be removed to keep the test focused on rendered behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/direction_test.gleam` around lines 56 - 59, Remove the low-signal helper round-trip assertion by deleting the call and expectation that use_direction returns direction; specifically remove the lines invoking ui_direction.use_direction(...) and the subsequent expect.to_equal comparing it to ui_direction.direction_rtl(...), and keep only the render-focused assertion that verifies the rendered direction (so retain the render check and expected ui_direction.direction_rtl reference).test/menubar_test.gleam (1)
102-134: 🧹 Nitpick | 🔵 TrivialConsider adding styled item mutator test for parity.
The headless behavior section includes an item mutator test (Lines 50-76) verifying variant/inset/disabled rendering, but the styled behavior section only tests menu config mutators. For completeness, consider adding a parallel styled item mutator test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/menubar_test.gleam` around lines 102 - 134, Add a parallel styled item-mutator test in the "styled behavior" describe block that mirrors the headless item mutator test: create a theme (theme.theme_default()), build a menu with ui_menubar.menubar_menu_item configured via the styled mutators (e.g., ui_menubar.menubar_menu_item_variant, ui_menubar.menubar_menu_item_inset, ui_menubar.menubar_menu_item_disabled, and ui_menubar.menubar_menu_item_attrs) and render it via weft_lustre.layout |> element.to_string, then assert the rendered string contains the expected data/aria attributes (e.g., data-variant="…", data-inset="true"/"false", aria-disabled="true" or equivalent) and any custom attribute like data-extra="1" so the styled tests have parity with the headless item mutator test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/carousel_test.gleam`:
- Around line 46-57: The test's assertion is too broad—replace the loose check
string.contains(rendered, "disabled") with a precise check that matches the HTML
attribute form rendered by the component, e.g. string.contains(rendered,
"disabled=\"\""), in the test for headless_carousel.carousel_previous (which
uses headless_carousel.carousel_control_config() |>
headless_carousel.carousel_control_disabled()); update the expectation
accordingly so the test explicitly verifies the disabled attribute format
instead of any occurrence of the substring "disabled".
In `@test/direction_test.gleam`:
- Around line 32-45: The LTR test (using direction_provider,
direction_provider_config, and direction_ltr) only asserts that rendered
contains 'dir="ltr"' but lacks the complementary negative assertion; add an
assertion that 'dir="rtl"' is not present (e.g., check that
string.contains(rendered, "dir=\"rtl\"") |> expect.to_equal(expected: False)) to
mirror the RTL tests and strengthen coverage.
In `@test/resizable_test.gleam`:
- Around line 31-33: The tests currently only assert that rendered contains
aria-orientation="horizontal" (via string.contains(rendered,
"aria-orientation=\"horizontal\"") |> expect.to_equal(True)); add explicit
negative assertions that the opposite orientation is not present by asserting
string.contains(rendered, "aria-orientation=\"vertical\"") |>
expect.to_equal(False) (or vice-versa if the test is for vertical) for each
occurrence (the checks around the rendered variable where expect.to_equal is
used on orientation — update the occurrences near the current assertions at
lines shown in the review). Ensure each block that asserts presence of one
orientation also asserts absence of the other using the same rendered value and
expect.to_equal(False).
In `@test/textarea_test.gleam`:
- Around line 86-87: Replace the broad substring assertion that looks for
"disabled" with an assertion for the exact attribute form your renderer emits
(change the string.contains(rendered_facade, "disabled") call to check the exact
token, e.g. ' disabled="disabled"' or ' disabled' depending on your renderer)
and keep using expect.to_equal(expected: True) for the positive case; also add a
complementary negative test that asserts the exact attribute is absent when
disabled is false (use string.contains(... ) |> expect.to_equal(expected: False)
or negate the check) so the test verifies attribute-specific presence and
absence rather than any substring.
---
Outside diff comments:
In `@test/direction_test.gleam`:
- Around line 56-59: Remove the low-signal helper round-trip assertion by
deleting the call and expectation that use_direction returns direction;
specifically remove the lines invoking ui_direction.use_direction(...) and the
subsequent expect.to_equal comparing it to ui_direction.direction_rtl(...), and
keep only the render-focused assertion that verifies the rendered direction (so
retain the render check and expected ui_direction.direction_rtl reference).
In `@test/menubar_test.gleam`:
- Around line 102-134: Add a parallel styled item-mutator test in the "styled
behavior" describe block that mirrors the headless item mutator test: create a
theme (theme.theme_default()), build a menu with ui_menubar.menubar_menu_item
configured via the styled mutators (e.g., ui_menubar.menubar_menu_item_variant,
ui_menubar.menubar_menu_item_inset, ui_menubar.menubar_menu_item_disabled, and
ui_menubar.menubar_menu_item_attrs) and render it via weft_lustre.layout |>
element.to_string, then assert the rendered string contains the expected
data/aria attributes (e.g., data-variant="…", data-inset="true"/"false",
aria-disabled="true" or equivalent) and any custom attribute like data-extra="1"
so the styled tests have parity with the headless item mutator test.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
docs/test_audit_headscratchers.mdtest/carousel_test.gleamtest/direction_test.gleamtest/form_test.gleamtest/input_otp_test.gleamtest/item_test.gleamtest/menubar_test.gleamtest/navigation_menu_test.gleamtest/resizable_test.gleamtest/textarea_test.gleam
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (18)
docs/test_audit_headscratchers.md (1)
1-63: Well-structured test audit documentation.The heuristics and recommendations provide clear, actionable guidance for improving test signal. The rewrite patterns (attribute extraction, negative assertions, event wiring, independent oracles) align well with testing best practices.
Minor note: The direction_test.gleam entry (Lines 47-49) mentions only a single
dir="rtl"assertion, but the actual rewritten test in this PR now includes the recommended negative assertion (dir="ltr"expected False). Consider updating this document to reflect the improvements already made, or clarify this is a pre-rewrite snapshot.test/form_test.gleam (2)
36-38: Good addition of semantic element check.Adding the
<formpresence check ensures the rendered output actually produces a form element, not just any container. This complements the facade parity assertion.
71-72: Improved assertions with specific id attribute checks.The
id="name"andid="bio"assertions verify that field configuration properly propagates to the rendered output. This is more specific than the previous semantic container check and aligns with the PR's goal of testing observable behavior.Also applies to: 88-90
test/item_test.gleam (3)
13-30: Good shift to rendering-based assertions.The test now verifies that config mutators (
item_variant,item_size) produce the expecteddata-variantanddata-sizeattributes in the rendered output, rather than relying on internal helper round-trips.
48-68: Styled test properly mirrors headless behavior verification.The styled variant correctly renders the component and asserts on the same data attributes, ensuring parity between headless and styled paths.
92-94: Good addition of item-title slot assertion.Verifying the
data-slot="item-title"marker ensures the nested component structure renders correctly.test/input_otp_test.gleam (2)
30-34: Excellent boundary validation with negative assertion.The test sets
length: 4, so valid indices are 0-3. Asserting thatdata-index="3"is present anddata-index="4"is absent verifies the length constraint is correctly enforced in the rendered output. This directly addresses the audit concern about robust length-specific behavior verification.
85-89: Consistent boundary check in styled behavior test.The styled variant mirrors the headless boundary validation, ensuring parity.
test/menubar_test.gleam (2)
50-76: Comprehensive item mutator rendering test.This test verifies that multiple item config mutations (destructive variant, inset, disabled) all render correctly in a single component. This consolidation improves maintainability while covering key state combinations.
95-96: Good shift to semantic ARIA role assertion.Checking
role="menubar"instead ofdata-slot="menubar"verifies the accessibility semantics, which is more meaningful for user behavior than internal slot markers.Also applies to: 155-156
test/navigation_menu_test.gleam (4)
13-30: Strong viewport disabled behavior test.This test verifies two important aspects when viewport is disabled:
data-viewport="false"flag is rendereddata-slot="navigation-menu-viewport"is absentThis directly addresses the audit concern about not verifying "viewport toggling behavior when disabled."
31-45: Good complementary test for default viewport behavior.Verifying both the data flag (
data-viewport="true") and slot presence ensures the default enabled state renders correctly. The pairing with the disabled test provides complete coverage.
85-86: Useful semantic attribute check.Asserting
type="button"on the trigger verifies proper button semantics, improving the test's ability to catch accessibility regressions.
108-112: Styled viewport test mirrors headless behavior correctly.The styled variant properly verifies both the data flag and slot absence when viewport is disabled, maintaining parity with headless tests.
test/carousel_test.gleam (2)
13-45: Good end-to-end orientation propagation test.Building the full carousel structure (root → content → item) with vertical orientation applied at each level and verifying
data-orientation="vertical"in rendered output is a strong improvement over config helper round-trips.Consider adding a negative assertion to verify
data-orientation="horizontal"is absent, ensuring the orientation was actually changed from any default.
97-139: Styled orientation test maintains parity with headless.The styled variant correctly mirrors the headless test structure, verifying orientation propagation through the themed component hierarchy.
test/resizable_test.gleam (1)
12-46: Good improvement: these tests now validate rendered behavior instead of mutator round-trips.This materially increases test signal and aligns with the PR goal.
Also applies to: 83-112
test/textarea_test.gleam (1)
20-30: Nice coverage upgrade for textarea facade parity and rendered attributes.The facade-vs-input equality checks plus rows/placeholder/disabled assertions significantly improve confidence in rendered behavior.
Also applies to: 44-47, 60-69
| it("direction_provider applies ltr dir attribute", fn() { | ||
| let rendered = | ||
| headless_direction.direction_provider( | ||
| config: headless_direction.direction_provider_config( | ||
| direction: headless_direction.direction_ltr(), | ||
| ), | ||
| children: [], | ||
| ) | ||
| |> weft_lustre.layout(attrs: []) | ||
| |> element.to_string | ||
|
|
||
| string.contains(rendered, "dir=\"ltr\"") | ||
| |> expect.to_equal(expected: True) | ||
| }), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a negative assertion for consistency.
The RTL tests include negative assertions verifying dir="ltr" is absent. For symmetry and stronger coverage, the LTR test should also verify that dir="rtl" is absent.
Suggested addition
string.contains(rendered, "dir=\"ltr\"")
|> expect.to_equal(expected: True)
+
+ string.contains(rendered, "dir=\"rtl\"")
+ |> expect.to_equal(expected: False)
}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("direction_provider applies ltr dir attribute", fn() { | |
| let rendered = | |
| headless_direction.direction_provider( | |
| config: headless_direction.direction_provider_config( | |
| direction: headless_direction.direction_ltr(), | |
| ), | |
| children: [], | |
| ) | |
| |> weft_lustre.layout(attrs: []) | |
| |> element.to_string | |
| string.contains(rendered, "dir=\"ltr\"") | |
| |> expect.to_equal(expected: True) | |
| }), | |
| it("direction_provider applies ltr dir attribute", fn() { | |
| let rendered = | |
| headless_direction.direction_provider( | |
| config: headless_direction.direction_provider_config( | |
| direction: headless_direction.direction_ltr(), | |
| ), | |
| children: [], | |
| ) | |
| |> weft_lustre.layout(attrs: []) | |
| |> element.to_string | |
| string.contains(rendered, "dir=\"ltr\"") | |
| |> expect.to_equal(expected: True) | |
| string.contains(rendered, "dir=\"rtl\"") | |
| |> expect.to_equal(expected: False) | |
| }), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/direction_test.gleam` around lines 32 - 45, The LTR test (using
direction_provider, direction_provider_config, and direction_ltr) only asserts
that rendered contains 'dir="ltr"' but lacks the complementary negative
assertion; add an assertion that 'dir="rtl"' is not present (e.g., check that
string.contains(rendered, "dir=\"rtl\"") |> expect.to_equal(expected: False)) to
mirror the RTL tests and strengthen coverage.
| string.contains(rendered, "aria-orientation=\"horizontal\"") | ||
| |> expect.to_equal(expected: True) | ||
| }), |
There was a problem hiding this comment.
Add negative orientation assertions to make these checks regression-resistant.
Line 31, Line 43, and Line 107 only assert the presence of aria-orientation="horizontal". These still pass if both orientations are emitted. Add explicit absence checks for the opposite orientation in each case.
Proposed test hardening
string.contains(rendered, "aria-orientation=\"horizontal\"")
|> expect.to_equal(expected: True)
+ string.contains(rendered, "aria-orientation=\"vertical\"")
+ |> expect.to_equal(expected: False) string.contains(rendered, "aria-orientation=\"horizontal\"")
|> expect.to_equal(expected: True)
+ string.contains(rendered, "aria-orientation=\"vertical\"")
+ |> expect.to_equal(expected: False) string.contains(rendered, "aria-orientation=\"horizontal\"")
|> expect.to_equal(expected: True)
+ string.contains(rendered, "aria-orientation=\"vertical\"")
+ |> expect.to_equal(expected: False)Also applies to: 43-45, 107-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/resizable_test.gleam` around lines 31 - 33, The tests currently only
assert that rendered contains aria-orientation="horizontal" (via
string.contains(rendered, "aria-orientation=\"horizontal\"") |>
expect.to_equal(True)); add explicit negative assertions that the opposite
orientation is not present by asserting string.contains(rendered,
"aria-orientation=\"vertical\"") |> expect.to_equal(False) (or vice-versa if the
test is for vertical) for each occurrence (the checks around the rendered
variable where expect.to_equal is used on orientation — update the occurrences
near the current assertions at lines shown in the review). Ensure each block
that asserts presence of one orientation also asserts absence of the other using
the same rendered value and expect.to_equal(False).
test/textarea_test.gleam
Outdated
| string.contains(rendered_facade, "disabled") | ||
| |> expect.to_equal(expected: True) |
There was a problem hiding this comment.
Make the disabled assertion attribute-specific.
Line 86 uses a broad substring ("disabled"), which can pass on unrelated text. Tighten this to the exact boolean attribute form your renderer emits, and add a negative assertion for a false/absent disabled state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/textarea_test.gleam` around lines 86 - 87, Replace the broad substring
assertion that looks for "disabled" with an assertion for the exact attribute
form your renderer emits (change the string.contains(rendered_facade,
"disabled") call to check the exact token, e.g. ' disabled="disabled"' or '
disabled' depending on your renderer) and keep using expect.to_equal(expected:
True) for the positive case; also add a complementary negative test that asserts
the exact attribute is absent when disabled is false (use string.contains(... )
|> expect.to_equal(expected: False) or negate the check) so the test verifies
attribute-specific presence and absence rather than any substring.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/textarea_test.gleam (1)
86-87:⚠️ Potential issue | 🟡 MinorAdd a complementary negative disabled assertion path.
Great improvement to exact attribute matching, but we still need an enabled-state assertion (
disabledabsent) to prevent one-sided regressions.Proposed follow-up test
@@ describe("styled textarea facade", [ it("styled/textarea forwards to styled/input and keeps attrs", fn() { @@ string.contains(rendered_facade, "disabled=\"\"") |> expect.to_equal(expected: True) }), + it("styled/textarea does not render disabled attribute when not disabled", fn() { + let t = theme.theme_default() + + let cfg = + ui_textarea.textarea_config( + theme: t, + value: "Hello", + on_input: fn(_value) { "changed" }, + ) + |> ui_textarea.textarea_rows(theme: t, rows: 5) + + let rendered = + ui_textarea.textarea(theme: t, config: cfg) + |> weft_lustre.layout(attrs: []) + |> element.to_string + + string.contains(rendered, "disabled=\"\"") + |> expect.to_equal(expected: False) + }), ]),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/textarea_test.gleam` around lines 86 - 87, Add a complementary negative assertion to the existing positive disabled check so tests also validate the enabled state; locate the string.contains(rendered_facade, "disabled=\"\"") |> expect.to_equal(expected: True) assertion and add a counterpart that asserts the absence of the attribute (e.g. string.contains(rendered_facade, "disabled=\"\"") |> expect.to_equal(expected: False) or an explicit negation) for the enabled-path scenario so both disabled and enabled regressions are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/carousel_test.gleam`:
- Around line 13-45: The test titled "vertical orientation is reflected on
root/content/item" only checks for a single data-orientation token in the
rendered string; either expand the assertions to explicitly check each node
(root produced by headless_carousel.carousel, content from
headless_carousel.carousel_content, and item from
headless_carousel.carousel_item for data-orientation="vertical") or rename the
test to accurately reflect its current scope (e.g., "vertical orientation is
present in rendered output"); update the assertion logic around
weft_lustre.layout |> element.to_string accordingly and keep the test name and
assertions consistent.
---
Duplicate comments:
In `@test/textarea_test.gleam`:
- Around line 86-87: Add a complementary negative assertion to the existing
positive disabled check so tests also validate the enabled state; locate the
string.contains(rendered_facade, "disabled=\"\"") |> expect.to_equal(expected:
True) assertion and add a counterpart that asserts the absence of the attribute
(e.g. string.contains(rendered_facade, "disabled=\"\"") |>
expect.to_equal(expected: False) or an explicit negation) for the enabled-path
scenario so both disabled and enabled regressions are covered.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
docs/test_audit_headscratchers.mdtest/carousel_test.gleamtest/textarea_test.gleam
📜 Review details
🔇 Additional comments (3)
docs/test_audit_headscratchers.md (1)
13-71: Solid audit rubric and prioritization.This is clear, actionable, and well-scoped for follow-up rewrites without over-blocking the current PR.
test/carousel_test.gleam (1)
46-57: Good upgrade to precise rendered-behavior checks.The explicit
disabled=""assertion and addedcarousel-content/carousel-previousslot checks materially improve test signal.Also applies to: 93-99, 187-193
test/textarea_test.gleam (1)
44-47: Attribute assertions are now much more precise.The move to exact
rows,placeholder, anddisabled=""checks is a solid signal improvement over broad token matching.Also applies to: 86-87
| it("vertical orientation is reflected on root/content/item", fn() { | ||
| let vertical = headless_carousel.carousel_vertical() | ||
|
|
||
| headless_carousel.carousel_orientation_is_vertical( | ||
| orientation: headless_carousel.carousel_content_config_orientation( | ||
| config: content_config, | ||
| ), | ||
| let view = | ||
| headless_carousel.carousel( | ||
| config: headless_carousel.carousel_config() | ||
| |> headless_carousel.carousel_orientation(orientation: vertical), | ||
| children: [ | ||
| headless_carousel.carousel_content( | ||
| config: headless_carousel.carousel_content_config() | ||
| |> headless_carousel.carousel_content_orientation( | ||
| orientation: vertical, | ||
| ), | ||
| children: [ | ||
| headless_carousel.carousel_item( | ||
| config: headless_carousel.carousel_item_config() | ||
| |> headless_carousel.carousel_item_orientation( | ||
| orientation: vertical, | ||
| ), | ||
| child: weft_lustre.text(content: "Slide 1"), | ||
| ), | ||
| ], | ||
| ), | ||
| ], | ||
| ) | ||
| |> expect.to_equal(expected: True) | ||
|
|
||
| headless_carousel.carousel_orientation_is_vertical( | ||
| orientation: headless_carousel.carousel_item_config_orientation( | ||
| config: item_config, | ||
| ), | ||
| let rendered = | ||
| weft_lustre.layout(attrs: [], child: view) | ||
| |> element.to_string | ||
|
|
||
| string.contains(rendered, "data-orientation=\"vertical\"") | ||
| |> expect.to_equal(expected: True) | ||
| }), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test title overstates what is currently asserted.
"vertical orientation is reflected on root/content/item" currently validates only a single data-orientation="vertical" token. Either assert orientation at each intended node or narrow the test name to match scope.
Proposed minimal fix (rename to match current assertion scope)
- it("vertical orientation is reflected on root/content/item", fn() {
+ it("vertical orientation is present in rendered markup", fn() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/carousel_test.gleam` around lines 13 - 45, The test titled "vertical
orientation is reflected on root/content/item" only checks for a single
data-orientation token in the rendered string; either expand the assertions to
explicitly check each node (root produced by headless_carousel.carousel, content
from headless_carousel.carousel_content, and item from
headless_carousel.carousel_item for data-orientation="vertical") or rename the
test to accurately reflect its current scope (e.g., "vertical orientation is
present in rendered output"); update the assertion logic around
weft_lustre.layout |> element.to_string accordingly and keep the test name and
assertions consistent.
1 participant
Motivation
Description
docs/test_audit_headscratchers.mdwhich documents heuristics for identifying low-signal tests, lists problematic tests, and recommends rewrite priorities and patterns.carousel_test.gleam,direction_test.gleam,form_test.gleam,input_otp_test.gleam,item_test.gleam,menubar_test.gleam,navigation_menu_test.gleam,resizable_test.gleam, andtextarea_test.gleam.data-orientation="vertical",data-viewport="false",data-disabled="true",aria-orientation="horizontal",role="menubar", presence ofid/rows/placeholder, and explicit negative assertions where appropriate).Testing
gleam testand verified the updated unit tests; the automated test suite succeeded.Codex Task