Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: add edge case tests for TitleBarComponent #452

Conversation

techvoyagerX
Copy link

Overview

This PR adds additional test coverage to TitleBarComponent in order to handle some edge cases that were previously untested. Specifically, it introduces tests for the following scenarios:

  1. Percent values:

    • Negative percentages (e.g., -0.5) should clamp to 0% width.
    • Percentages over 1 (e.g., 1.5) should clamp to 100% width.
  2. CSP Nonces:

    • Empty nonces for img, style, or script are now tested to ensure proper rendering.
    • Tests for the component's behavior when nonces are missing entirely.
  3. Missing inner_block:

    • Verified that the component renders gracefully even if no inner_block is provided.

This adds robustness and prevents edge cases from slipping through, especially in production environments where edge inputs might lead to unexpected behavior.

Changes Made

  • Expanded test cases in test/phoenix/live_dashboard/components/title_bar_component_test.exs to cover edge scenarios for percent, csp_nonces, and inner_block.
  • Minor adjustment to an assertion to fix a potential typo (div class="test-class" should be <div class="test-class">).

Looking forward to feedback! 🚀

@josevalim
Copy link
Member

Thank you for the PR. I'd prefer if out of range percentages raise. Also, we should treat only missing keys and nils as missing nonces, we shouldn't be checking for empty strings. If you would like to update the PR with code and tests, we will gladly merge it.

@SteffenDE
Copy link
Contributor

Hi @techvoyagerX,

I‘m a little bit confused about your contribution. You seem to have misunderstood the regex match on the div, which was perfectly fine before. Also, you added tests that don’t even pass and, to my own surprise, adapted test code for a component that doesn’t actually seem to be used. So, if you don’t mind: what was your intention behind this contribution and the corresponding issue (#451)?

I will close this for now, but please don’t mind explaining your intention, as I’m really curious. Also, if you really want to contribute, looking into the unused component and potentially removing it would be a welcome contribution.

@SteffenDE SteffenDE closed this Sep 30, 2024
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.

3 participants