Skip to content

test: assert exact ordered errors in the upstream parity table, not containment #33

Open

Description

@sugat009

What's wrong

tests/upstream_parity.rs ports all 31 rows of upstream's LINTER_FIXTURE_PARAMS as
(message, expected success, expected errors). The third column is an ordered list, but the test
only checks that each expected error is somewhere in cocox's output:

// tests/upstream_parity.rs:161-167
        if !expected_errors.is_empty() {
            for expected in *expected_errors {
                if !result.errors.iter().any(|e| e == *expected) {
                    failures.push(format!("{message:?}: missing upstream error {expected:?}"));
                }
            }
        }

That proves no upstream error is missing. It does not constrain:

  • Order. cocox prints errors in the order validate_detailed_pattern collects them
    (src/validators.rs:59-71), and that order is user-visible.
  • Extra errors on rows that already fail. An extra error on a row upstream expects to pass
    does get caught, because the success column flips. An extra error on a row upstream expects to
    fail is invisible to this assertion.

This landed with #28. On main the third column is discarded entirely
(for (message, expected_success, _) in ...), so #28 is a large improvement; this is the last step.

Evidence

Order is user-visible and currently matches upstream:

$ commitlint "invalid (test): add new feature"      # upstream 2.0.0
- Invalid type 'invalid'. Type must be one of: build, ci, docs, feat, fix, perf, refactor, style, test, chore, revert, bump.
- There cannot be a space after the type.

$ cocox "invalid (test): add new feature"           # PR head 9d3a672, identical
- Invalid type 'invalid'. Type must be one of: build, ci, docs, feat, fix, perf, refactor, style, test, chore, revert, bump.
- There cannot be a space after the type.

Two regressions that containment misses and equality catches, both reproduced in a scratch copy of
the tree at this head:

  1. Reordering. Swap validate_commit_type and validate_commit_type_no_space_after in the
    array at src/validators.rs:59-71. matches_upstream_linter_fixtures passes. Equality fails:

    "invalid (test): add new feature":
      upstream ["Invalid type 'invalid'. ...", "There cannot be a space after the type."]
      cocox    ["There cannot be a space after the type.", "Invalid type 'invalid'. ..."]
    
  2. A cocox-only extra error on a failing row. Add a validator that also reports
    Description must have a leading space. when the description is missing and the colon has no
    trailing space. matches_upstream_linter_fixtures passes. Equality fails:

    "feat(test):":
      upstream ["Description is missing."]
      cocox    ["Description is missing.", "Description must have a leading space."]
    

Both regressions are the shape a well-intentioned refactor produces, and neither changes any
pass/fail verdict.

Tightening costs nothing today

Strict ordered equality over all 31 rows passes at this head. Verified by adding the assertion to a
copy of the tree and running the suite: upstream_parity 3 passed, and the whole suite stayed green
(60 + 111 + 3 + 11 + 1 + 3 unit and integration tests, 0 failed).

What "done" looks like

Replace the containment loop in matches_upstream_linter_fixtures with:

        let expected: Vec<String> = expected_errors.iter().map(|e| (*e).to_string()).collect();
        if result.errors != expected {
            failures.push(format!(
                "{message:?}: upstream errors {expected:?}, cocox {:?}",
                result.errors
            ));
        }
  • The if !expected_errors.is_empty() guard goes away. It is harmless today, because Valid,
    Ignored and Empty all return errors: vec![] by construction in
    lint_commit_message_with_errors, but equality makes that structural rather than incidental.
  • Update the doc comment on the test: it currently says failed rows must contain all upstream
    error strings.
  • Leave fixture_table_has_all_upstream_rows (tests/upstream_parity.rs:134-141) in place. It
    guards the row count independently and equality does not subsume it.
  • Per AGENTS.md, do not add local rows to this file. Any new input belongs in tests/cli.rs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    • Status
      Todo

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions