Closed Bug 2008282 Opened 2 months ago Closed 2 months ago

selectors/parser tests should use assert_eq instead of matches! when checking concrete Component values

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement

Tracking

()

RESOLVED FIXED
148 Branch
Tracking Status
firefox148 --- fixed

People

(Reporter: nabeyang, Assigned: nabeyang)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/143.0.0.0 Safari/537.36

Steps to reproduce:

In the selector parser tests within servo/components/selectors/parser.rs, I replaced assertions that used matches! with assert_eq! where the test was already matching both the variant and the concrete value.

Originally, patterns like:

    assert!(matches!(
        iter.next(),
        Some(&Component::PseudoElement(PseudoElement::Before))
    ));

were present, even though the value inside the pattern is fully specified.

I updated these to explicit equality checks, for example:

    assert_eq!(
        iter.next(),
        Some(&Component::PseudoElement(PseudoElement::Before))
    );

For LocalName, I also replaced the matches! with an assert_eq! including concrete field values.

Actual results:

The tests used matches! macros while already specifying exact values inside the pattern, which means they were not leveraging the true intent of matches! for pattern-only checks.

This results in:

  • Redundant use of matches! when concrete value comparison is desired.
  • Less helpful failure output compared to assert_eq!, since matches! does not show expected vs actual on test failure.

The current practice could cause confusion about whether only the variant or the concrete value is being tested.

Expected results:

Tests should use assert_eq! when they intend to verify both the variant and the exact value of the component returned by the iterator.

Using assert_eq!:

  • Makes the intent explicit.
  • Produces better failure messages with diff output.
  • Avoids misleading use of matches!, which is primarily for pattern shape checks without caring about concrete fields.

If only the variant matters, matches! should continue to be used, but in these cases the concrete value was relevant.

In the selector parser tests (servo/components/selectors/parser.rs), replace use of the
matches! macro with assert_eq! when the intent is to verify both the variant and
the exact value of a returned Component.

Prior to this change, several tests were written like:

    assert!(matches!(
        iter.next(),
        Some(&Component::PseudoElement(PseudoElement::Before))
    ));

Even though the pattern includes a specific value, using matches! only asserts
that the pattern shape matches, and does not provide helpful diagnostic output
on failure.

Using assert_eq! for these cases makes the intent explicit and produces clearer
expected vs actual output when tests fail. This change improves readability
and debuggability of the tests without altering their semantics.

Where only the variant shape matters (no concrete fields to check), matches! can
still be appropriate. But in the affected cases, the concrete value was already
being specified, so assert_eq! is a better fit.

Assignee: nobody → nabeyang
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Pushed by ealvarez@mozilla.com: https://github.com/mozilla-firefox/firefox/commit/7414b9348184 https://hg.mozilla.org/integration/autoland/rev/03c93aecc032 selectors/parser tests should use assert_eq instead of matches! when checking concrete Component values r=emilio,firefox-style-system-reviewers
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 148 Branch
QA Whiteboard: [qa-triage-done-c149/b148]
You need to log in before you can comment on or make changes to this bug.